In August, I wrote about what the native Delta read path does: why Comet turned away every Delta table, and how a Delta query can reuse Comet’s parquet scan with deletion vectors (the bitmaps Delta uses to mark rows deleted without rewriting the file) applied inside it. This one is about what it took to get it accepted.
apache/datafusion-comet#5365 opened on August 15 and merged on October 13, 58 days later. Along the way, the PR collected 336 comments between the review threads and the conversation, and nine smaller PRs came out of it and merged on their own, eight in Comet and one in DataFusion. Nearly every review round turned up a problem, and the version that landed is much safer than the one I opened.
The Shape of It
The easiest way to tell this story is by size.
Lines added versus main at each pushed version of #5365. The notch on September 3 is the split and the calendar rebase arriving on one day.
It opened at about 3.9K lines. A week later, after Chao Sun’s first review rounds, it stood at 8.5K. It passed 10K on August 27 and peaked at 16.1K on September 2. The next morning it fell to 13.4K, the only meaningful drop, and within about twelve hours it was at 17.5K, above the peak it had just left. From there, it climbed to 21.5K at merge.
Almost all of those moves began with someone identifying problems through the PR review process.
Building on What Was Already There
A contrib module is an optional part of a project that lives in its own directory and only gets built when you ask for it, so format-specific code stays out of the core. Scott Schenkein had been building Comet’s contrib infrastructure, and his #4952, which added the build gate and wiring, merged on August 19.
Chao’s first comment, on August 18, asked me to build on #4952 and to land the work in smaller pieces, one feature at a time. I pushed back: the features weren’t independent, and several safety checks only existed because deletion vectors, column mapping (Delta’s feature that lets a table rename a column without rewriting its files) and Delta’s own test suites had run together. He agreed the reader, its checks and the essential tests should stay together, with one request: it had to be explicitly opt-in, meaning off unless you turn it on, so nobody gets the new reader by surprise.
By August 20, the code lived in contrib/delta-spark and used #4952’s CometScanContrib hook in place of my own, which is why the line count dipped that day. Two days later, the flag became spark.comet.scan.delta.enabled.
Review Rounds That Found Real Bugs
Then came the correctness work. Chao ranked each finding by priority and usually attached a small reproduction against stock Spark and Delta, so there was never any doubt a problem existed. His first round, on August 20, had eight findings. Among them: a filter whose value comes from a subquery being applied before a LIMIT instead of after it; shallow clones (Delta tables that point at another table’s files instead of copying them) whose files live in more than one storage bucket; and the memory needed to hold deletion vectors.
Most fixes took one form: if the native reader couldn’t prove it handled a case, it declined it, meaning Comet hands that scan back to Spark’s own reader, slower but correct, and a regression test pins the behavior. One finding stood out. A column-mapping rename could make two columns collide, so a query that should have returned (1,100) returned (100,100). No error, no crash, just a wrong row. Chao put it at the top of the list.
Another was about Unicode: Spark matches column names case-insensitively using Java’s lowercasing rules, which differ from Rust’s for some characters. Matching Java exactly took a case-folding layer with generated lookup tables, and that is much of the climb from 10K to 16K.
The Split
On September 2, Andy Grove built the branch and reviewed the parts touching Comet’s core. He counted about 2,600 lines of core changes unrelated to Delta, and made a point I couldn’t argue with: “A future bisect on a Parquet field-matching regression would also land on a 16k-line Delta commit.” (A bisect walks the history commit by commit to find where a bug started.) He measured the Java-exact name matching at 19x to 61x slower per file and found each scan carrying its own 18,801-byte copy of the case tables in the serialized plan. He also checked my Rust port of Java’s Greek sigma rule over 212,518 strings: zero mismatches. The issue wasn’t correctness. It was cost, and where the code belonged.
The timing helped. Oleks V’s #5602 merged on main that same day with Unicode-aware field-name matching. So I dropped my case-folding stack entirely and moved the other core pieces into their own PRs: #5653 for three small fixes and #5654 for Spark’s handling of duplicate fields and field ids (numeric labels parquet can attach to each column, so a reader can match columns after a rename). That’s the drop to 13.4K.
The Nine-Day Shift
It didn’t stay down. On September 3, Chao found that a date of 1500-01-01, written in Spark’s LEGACY calendar mode, came back as 1500-01-10 through the native Delta path. Spark 2 used the old hybrid Julian and Gregorian calendar, which disagrees with the plain Gregorian one for anything before October 1582. Spark 3 uses Gregorian throughout unless told to write the old way (LEGACY mode), so a reader has to convert those old values on load. Old timestamps need the same treatment, with a cutoff in 1900.
The fix was a new native file, datetime_rebase.rs, about 2,700 lines, followed by four more review rounds over the next few hours on cutoffs, timestamp encodings and nested columns. Chao then approved it.
September 3 also brought an answer to Oleks’s request for a test on an actual Delta workload rather than a sandbox. Erik Bogado ran it on public NYC taxi data: the native scan matched stock Spark (0.780s against 0.775s median) and removed every deleted row inside the scan.
Where a Contrib Ends
Liang-Chi Hsieh’s review on September 3 raised the question underneath all of this. About 1,060 lines of the main Scala support file existed to mirror how Hadoop resolves S3 credentials, and the calendar rebase fixed a gap plain parquet reads have too, but only behind a Delta flag. In his words, “this PR is paying, inside a Delta contrib, for two holes in Comet’s own read path.”
That didn’t need solving inside #5365, but it needed a home, and it became follow-up issues such as #5662 for the regular scan’s S3 options. On September 9, Andy confirmed the split had resolved his five points and made the maintainer call on packaging: the delta cargo feature stays in the default build. A cargo feature is a compile-time switch for Rust code, so the reader ships compiled in, at about 82 KB, while the runtime flag still decides whether it runs.
Chao’s later 11-point design pass on shared scan interfaces went the same way: seven tracked issues, and an approval with them on file.
The Long Tail
From mid-September, the findings narrowed. Andy reproduced a memory pool failure caused by giving every file with deletion vectors its own memory consumer, so they now share one per partition. Both reviewers caught the reader choosing its timestamp handling from how the file stored the value rather than from the type the query asked for. Chao went past reviewing and contributed the per-split deletion vector selectors, as a PR against my branch.
Before it merged upstream, someone had already ported it into their own Comet build.
Andy approved on October 11 after running 863 native tests and the Delta suites twice locally: “It’s a great base to build on.” Early on October 13 he fixed the last merge conflicts, and the PR went through the merge queue (GitHub’s final step that tests a PR against the latest main before merging it) at 02:38 UTC.
What It Turned Up Elsewhere
A new reader makes a good bug detector for the code around it. These merged as separate PRs.
Split out during review
- #5653: three core fixes that had been riding along.
- #5654: field lookup that follows Spark’s rules for duplicate fields and field ids, rebuilt on top of #5602.
Parquet field ids, a chain that grew out of #5654
- #6116: reject a file with no field ids at any depth, as Spark does.
- apache/datafusion#25895: a DataFusion backport so that converting INT96 timestamps (an older parquet timestamp encoding) no longer drops the field id on the struct that holds them.
- #6862: a Comet test for that case.
Storage credentials, from issues I filed while comparing the Delta reader’s checks with the rest of Comet
- #6059: Azure authentication resolves through Hadoop, and stray
AZURE_*environment variables can no longer override configured credentials. - #6065: the Iceberg scan reads its supported storage schemes from the native side instead of keeping three lists that disagreed.
Calendar rebase
- #6559: a rebase refused under
EXCEPTIONmode now raises Spark’s ownSparkUpgradeExceptioninstead of a generic native error. The Delta scan’s rebase path exposed it.
Test infrastructure
- #6887: the S3 suites test against MinIO, a local server that speaks the S3 protocol, and its image could no longer be pulled. The Delta MinIO job hit this on October 12, so the fix had to land first.
Staying With It
What kept this moving was treating each finding as something to answer with a test, not a position to defend. When Chao found a gap, the response was a reproduction, a fix or a decline, and a regression test. When Andy measured a cost, arguing correctness would have missed the point, since his own check had confirmed it. What mattered was whether core should pay for it, and once #5602 existed, the answer was no.
If you’re trying to land a large change in an Apache project, here’s what I’d pass on:
- Split along the lines reviewers need. Keep together what can’t be correct apart, and pull out anything a future bisect shouldn’t have to wade through.
- Decline when unsure. Falling back to Spark costs speed; a wrong row costs trust. A conservative gate is far easier to approve than a clever one.
- Turn scope debates into issues. This PR produced fourteen tracking issues, each a discussion that didn’t have to happen inside it.
- Fix the neighbors separately. Bugs you find next door make small PRs, and they shrink the big one.
- Keep merging main. Conflicts showed up until the final day, including one from my own Azure fix, #6059, two days before the merge.
Thanks
To Scott Schenkein, whose earlier Delta contrib (#4366) gave this one its module shape, its decline-first approach and its test catalog, and whose #4700 and #4952 built the infrastructure it sits on. To Aditya Vaish, who spotted the core idea first in #4669: reuse Comet’s native parquet scan for Delta. Both are co-authors on the merged commit.
To Chao Sun and Andy Grove for the reviews. Chao approved it twenty times as it kept changing under him, and Andy carried it over the line himself.
And to Liang-Chi Hsieh, Oleks V and Erik Bogado, for the architecture review, the call for a real workload (plus #5602), and the workload itself.
What’s Next
It should ship in Comet 1.2.0 (release planning is in #6550). Until then, you can build it from main as described in the August post. When 1.2.0 ships, I’ll link it here.