SEPURI-SAI-KRISHNA commented on code in PR #17656:
URL: https://github.com/apache/iceberg/pull/17656#discussion_r3789715007
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestDictionaryRowGroupFilter.java:
##########
@@ -462,12 +462,17 @@ SCHEMA, notStartsWith("no_stats",
UUID.randomUUID().toString()))
shouldRead =
new ParquetDictionaryRowGroupFilter(SCHEMA,
notStartsWith("some_nulls", "some"))
.shouldRead(parquetSchema, rowGroupMetadata, dictionaryStore);
- assertThat(shouldRead).as("Should skip: no match in dictionary").isFalse();
+ assertThat(shouldRead).as("Should read: null values do not start with the
prefix").isTrue();
shouldRead =
new ParquetDictionaryRowGroupFilter(SCHEMA, notStartsWith("no_nulls",
"xxx"))
.shouldRead(parquetSchema, rowGroupMetadata, dictionaryStore);
assertThat(shouldRead).as("Should read: dictionary contains a matching
entry").isTrue();
+
+ shouldRead =
+ new ParquetDictionaryRowGroupFilter(SCHEMA, notStartsWith("no_nulls",
""))
Review Comment:
You're right, that assertion passes with or without the fix. `no_nulls` has
no nulls, so the new guard never fires and it falls straight through to the
dictionary loop, exactly as before.
It's there as the complement rather than as the regression test: it pins the
guard to `mayContainNulls` specifically, so a future change that returns
`ROWS_MIGHT_MATCH` unconditionally (and quietly disables dictionary skipping
for `notStartsWith` altogether) fails here.
The real regression test is the `notStartsWith("some_nulls", "some")`
assertion above, as you guessed. I confirmed it by reverting just the main-code
change locally, that one assertion fails on both `PARQUET_1_0` and
`PARQUET_2_0`, and it's the only failure.
Happy to add a comment on the `no_nulls` case spelling that out if you think
it reads as redundant coverage otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
