Addresses #658

[jkumz]

Linked issue #658

Fixes a corruption path in partial string chunk scans by separating dictionary materialization from row-index assignment.
The old path reused DictionaryColumn::scan for both ValueVector and StringChunkData results, but those callers treated the second scan coordinate differently: output row position for ValueVector, destination dictionary slot for StringChunkData. The fix keeps the ValueVector scan path intact, adds a StringChunkData materialization helper that returns the actual old-index to new-index mapping, and has StringColumn write result row indexes only after that mapping is known.

[adsharma]

Amended commit to pass clang-format lint step.

[adsharma]

Looks good.

Only concern is the size of the parquet files that are getting added to the test dir. Would prefer that they're generated.

[jkumz]

Looks good.

Only concern is the size of the parquet files that are getting added to the test dir. Would prefer that they're generated.

I tried generating these to avoid checking in the node parquet files, but the generated versions do not preserve the regression signal.

The node tables are logically simple (id = 1..34747, key = node_{id}), but the bug only reproduces with the original parquet import layout. I tested:

• Inline-generated nodes via UNWIND range(...) + pre-fix binary: passes, so it does not reproduce the pre-fix failure
• Ladybug-generated node parquets via COPY ... TO parquet + pre-fix binary: passes, so it does not reproduce the pre-fix failure
• Repo-native CSV_TO_PARQUET(...) generated node parquets via CSVConverter + pre-fix binary: passes, so it does not reproduce the pre-fix failure

So replacing these with generated logical equivalents would make this test stop catching the bug. The fixtures are from an internal graph that was minimized and obfuscated; the delete/reload sequence deterministically reproduces a silent persisted relationship-property corruption issue. Given the severity of the bug class, I think keeping the ~660KB parquet fixtures is the safer tradeoff than weakening the regression test.

If the main concern is binary blobs in git rather than test size, I can add a README note explaining why these specific parquet bytes are required?

[adsharma]

but the bug only reproduces with the original parquet import layout

I still don't understand what was special about the checked in parquet files. How were they generated?

[jkumz]

but the bug only reproduces with the original parquet import layout

I still don't understand what was special about the checked in parquet files. How were they generated?

They were not generated from a standalone recipe. They are minimized/obfuscated artifacts from a larger internal-data repro. There was a use case in a larger graph with internal data that despite all read queries, ingestion logic, etc all being correct worked fine but after restarting the db no longer worked. So then I reduced that down into a much more narrow set of nodes/rels that it happened on deterministically and investigated from there. Then I narrowed it down to this bug, obfuscated the data and kept only the bare minimum that kept hitting the bug, extracted from the database into Parquet and went from there. That's the fixtures currently checked in.

[adsharma]

@jkumz looks like the generated parquet files are good enough to reproduce the problem. The following tests fail after reverting the fix.

	2780 - StringChunkScanTest.PartialAndFullStringChunkScansMatchValueVectorScans (Failed)
	2781 - StringChunkScanTest.PartialStringChunkScansPreserveIndexInvariantsWhenAppending (Failed)
	2782 - StringChunkScanTest.JsonStringChunkScansMatchValueVectorScans (Failed)

Having a specially constructed parquet file that we can't independently reproduce is a problem (both in terms of size and reproducibility).

Commit f0e5f6a is passing tests and I'm ready to merge.

[adsharma]

But your point is that AnonymousParquetDeleteReload passed at commit 5cedc01 which is due to the specially constructed parquet file problem.

Since parquet format is a well documented open and immutable format, it should be possible to independently construct it from a script.

[adsharma]

If we determine that it's too expensive to construct the parquet in a script, an alternative is to place the parquet files in this repo: https://github.com/LadybugDB/dataset, so the main repo size is not affected.

[jkumz]

If we determine that it's too expensive to construct the parquet in a script, an alternative is to place the parquet files in this repo: https://github.com/LadybugDB/dataset, so the main repo size is not affected.

Ok fair enough, I'll give it another go later today if no luck I'll move it to that repo.

[jkumz]

Locally I reworked the fixtures so all three parquet files (A, B, and A_TO_B) are generated at test/CI time instead of being checked in. The e2e only reproduces with the original minimized node insertion order (sequential 1..N node tables do not trigger the failure), so the generator reconstructs that layout from compact zlib payloads under scripts/data/dictionary_bug/ rather than embedding opaque parquet blobs.

Relative to the original approach of checking in all three parquets (~652 KB), the tracked footprint is now ~100 KB (payloads + script) while CI still materializes the full ~669 KB of parquet on disk before tests. Compared with the intermediate approach of generating only sequential A/B and keeping A_TO_B checked in (~8 KB tracked), this is larger in git, but it preserves the e2e regression signal that the sequential generator lost.

Does that size tradeoff look acceptable, or would you rather move the payloads to the dataset repo?

[adsharma]

100k is comparable to what we have in the test dir. How about we move the insertion order data to be closer to where the generated parquet files live (test_files/dictionary_bug?).