fix: don't return garbage values instead of null under arrow.enable_null_check_for_get=false#187
Conversation
mkaufmann
force-pushed
the
fix/null-check-for-get-iceberg
branch
from
bbfddc7 to
e3c304c
Compare
mkaufmann
left a comment
mkaufmann
left a comment
There was a problem hiding this comment.
Review summary — 7 findings (3 Blocking · 1 Suggested · 1 Optional summary-only; 2 inline). Findings 1–5 are summary-only because they reference files outside the diff hunks; 6 and 7 are inline.
Blocking
1. PR body claims :jdbc-core:test passes with the fix, but 10 tests still fail at PR HEAD.
Running ./gradlew :jdbc-core:test --rerun-tasks against e3c304c produces 10 failures: 3× TimeStampVectorAccessorTest (testTimeStampMilliVector, testTimeStampMilliTZVector, testGetObjectWithUnsupportedType), 3× TimeZoneIntegrationTest (testInvalidTimezoneHandling, nullTimestampTZParameterStoresSqlNull, testGetObjectEqualsGetTimestamp — expected: null but was: 4714-12-31T23:52:58.000), 2× JDBCReferenceTest (select NULL::timestamp, select NULL::timestamptz), 1× DataCloudArrayTest::testGetArrayWithIndexPlusCountExceedingArraySize, 1× DataCloudConnectionFunctionalTest::testNetworkTimeoutPropagatesToServer. Either the diff is incomplete or the test-plan claim is wrong. Findings 2 and 3 break this down.
2. Timestamp accessors are still broken under flag-off — fix is incomplete.
jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/accessor/impl/TimeStampVectorAccessor.java
The PR body states that TimeStamp and TimeStampTZ "read holder.isSet, populated by vector.get(int, holder) which always honors validity regardless of the flag". The 6 failing TimeStamp tests above contradict that — null TIMESTAMP/TIMESTAMPTZ rows surface as 1970-01-01T00:00:00Z or 4714-12-31T23:52:58.000 (sentinel). TimeStampVectorGetter.createGetter does use the holder pattern the body cites as safe, but for TimeStamp* vectors that pattern is not actually flag-independent in Arrow 17. Apply the same IntPredicate nullChecker pattern to TimeStampVectorAccessor and TimeStampTZVectorAccessor — source wasNull from vector.isNull(row) rather than holder.isSet. Tighten the body's "all holders are validity-correct" claim to "all non-TimeStamp holders".
3. DataCloudArray exposes Iceberg-mode garbage for null elements inside VarChar/VarBinary/FixedSizeBinary/TimeStamp arrays.
jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/accessor/impl/DataCloudArray.java:61
extractDataFromVector calls dataVector.getObject(absIndex) per element. For VarCharVector / VarBinaryVector / FixedSizeBinaryVector (and the TimeStamp* variants per finding 2), getObject is also flag-gated under flag-off. DataCloudArrayTest::testGetArrayWithIndexPlusCountExceedingArraySize fails at PR HEAD with expected: null but was: "" — the bug philosophy of this PR ("source wasNull from the validity buffer") applies here too. Replace dataVector.getObject(absIndex) with a validity-aware lookup: if dataVector.isNull(absIndex) set the slot to null, otherwise call getObject.
Suggested
4. *FromNulled* accessor tests never assert wasNull() was set correctly.
jdbc-core/src/test/java/com/salesforce/datacloud/jdbc/core/accessor/impl/VarCharVectorAccessorTest.java:65 (and the equivalents in BinaryVectorAccessorTest)
The fix's full contract is "getBytes / getString / getObject set wasNull=true on a null row even when arrow.enable_null_check_for_get=false". testGetStringGetObjectAndGetObjectClassFromNulledVarCharVector and the binary equivalents only assert that the returned value is null. wasNull() is set as a side effect of getBytes() in production code, but the tests never read it back — QueryJDBCAccessorAssert.wasNull() exists and is unused. A regression that fixed the return value but broke wasNull would slip past the unit tests. Append a wasNull assertion after each getter call in each *FromNulled* test — one line per test.
Optional
5. nulledOutVector reproduces a degenerate variant, not the realistic Iceberg failure mode.
jdbc-core/src/test/java/com/salesforce/datacloud/jdbc/util/RootAllocatorTestExtension.java:150-153
nulledOutVector calls vector.clear() then setValueCount(original). clear() releases the buffers; setValueCount then allocates fresh zeroed buffers, so vector.get(row) reads start=end=0 and returns an empty byte[] — enough to make the current *FromNulled* tests fail without the fix, but not the production failure mode. The realistic case is "row N is valid (writes real bytes), row N+1 is set null via setNull(N+1) (validity bit cleared, buffer bytes left over), getter returns N's stale bytes for N+1". Add a parameterised test that builds a vector with mixed valid/null entries via setSafe + setNull and walks the accessor across all rows asserting null / wasNull()==true for the null rows. Pins the realistic Iceberg scenario.
Generated by the review-pr-tavern skill — a human did not write this comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #187 +/- ##
============================================
+ Coverage 80.78% 80.95% +0.17%
+ Complexity 1727 1723 -4
============================================
Files 123 123
Lines 5011 5042 +31
Branches 528 523 -5
============================================
+ Hits 4048 4082 +34
+ Misses 730 728 -2
+ Partials 233 232 -1
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
mkaufmann
changed the title
mkaufmann
changed the title
mkaufmann
changed the title
mkaufmann
changed the title
Iceberg sets -Darrow.enable_null_check_for_get=false on the JVM for performance. With that flag off, Arrow's VarCharVector/VarBinaryVector/FixedSizeBinaryVector .get(int) skip the validity check and return stale buffer bytes (typically an empty byte[]) for null rows instead of null. Both accessors were inferring wasNull from the returned value, so a SQL NULL came through as an empty string and wasNull() returned false. Consult vector::isNull directly. Run :jdbc-core:test with the flag off so the existing nulled-vector tests cover the Iceberg scenario.
…turn The Iceberg flag (arrow.enable_null_check_for_get=false) only affects the typed primitive vector.get(int) accessors on VarCharVector/VarBinaryVector/ FixedSizeBinaryVector. DecimalVector.getObject and ListVector/ LargeListVector.getObject unconditionally consult isSet, so the return-value-sniff pattern is safe here. Note that contract inline so the asymmetry with the just-fixed VarChar/Binary accessors doesn't read as a bug.
The earlier fix only covered VarChar/Binary because Arrow 17 only flag-gates the VarChar/Binary/FixedSizeBinary/TimeStamp* paths. The contract Arrow exposes is "skip the validity check on get", which authorises future minor versions to extend the gating to any vector type without breaking the API. A partner that updates Arrow on its own classpath would then silently regress against jars built here. Switch every accessor to source wasNull from vector.isNull(int), which reads the validity buffer directly and is not flag-gated. Covers TimeStamp, TimeStampTZ, BaseInt, Boolean, Float, Double, Date, Time, Decimal, the List accessors, and DataCloudArray's per-element extraction.
Each accessor was holding an IntPredicate that always pointed at the same vector's isNull. Inline `vector.isNull(row)` and store the vector directly, matching how Boolean/Float/Double already did it.
Move arrow.enable_null_check_for_get=false from :jdbc-core's tasks.test to the shared java-base-conventions tasks.withType<Test>().configureEach block, so jdbc, integration-test, jdbc-http, spark-datasource-core, and spark-datasource exercise the Iceberg scenario too. ./gradlew test passes across the full stack. Tighten the VarCharVectorAccessor comment to match BinaryVectorAccessor's wording — the buffer bytes between offsets aren't guaranteed empty.
mkaufmann
force-pushed
the
fix/null-check-for-get-iceberg
branch
from
5f32a6f to
6426a2c
Compare
3 participants
Summary
When Iceberg is on the classpath it sets
-Darrow.enable_null_check_for_get=falseon the JVM for performance. With that flag off, Arrow'sVarCharVector,VarBinaryVector,FixedSizeBinaryVector, andTimeStamp*Vectorgetters skip the validity check and return stale buffer bytes (often empty, but not guaranteed) for null rows instead ofnull. A SQL NULL surfaced as""/0/ a sentinel timestamp, andwasNull()returnedfalse. For exampleSELECT NULL::timestamp/timestamptzreturned1970-01-01T00:00:00Z(or the4714-12-31sentinel forgetObject); now - with the fix -returnsnull.Behavior under the flag's default (
true) is unchanged.Approach
Every accessor sources
wasNullfromvector.isNull(int)rather than fromholder.isSetor a return-value sniff.isNullreads the validity buffer directly viaBitVectorHelper— there's no per-row work for the flag to disable, so it is flag-proof.This is a wider migration than the bug strictly requires. The flag-gating is currently only on VarChar/Binary/FixedSizeBinary/TimeStamp* in Arrow 17, but Arrow's contract ("skip the validity check on get") authorizes future minor versions to extend the gating to any vector type. Sourcing
wasNulluniformly fromisNullinsulates us from any Arrow upgrade — including upgrades in consumer setups, where a tripwire test in this repository wouldn't fire.DataCloudArray.extractDataFromVectorhad the same bug for VarChar/Binary/TimeStamp inner element types; it now consultsdataVector.isNull(absIndex)before eachgetObjectcall.Test plan
arrow.enable_null_check_for_get=falseis set inbuildSrc/src/main/kotlin/java-base-conventions.gradle.ktstasks.withType<Test>().configureEach, so every test task across the build runs under the Iceberg scenario.main, 25 tests fail across 10 classes —JDBCReferenceTest,VarCharVectorAccessorTest,BinaryVectorAccessorTest,TimeStampVectorAccessorTest,DataCloudArrayTest,TimeZoneIntegrationTest, plus metadata/connection paths assertingexpected: null but was: "".