hudi-agent commented on code in PR #20298:
URL: https://github.com/apache/hudi/pull/20298#discussion_r4255192029
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/source/stats/FileStatsIndex.java:
##########
@@ -306,10 +306,12 @@ private static List<RowData> foldRowsByFiles(
List<Tuple3> columnStats =
alignedColumnRowsMap.values().stream().map(row -> {
if (row == null) {
- // NOTE: Since we're assuming missing column to essentially contain
exclusively
- // null values, we set null-count to be equal to value-count
(this behavior is
- // consistent with reading non-existent columns from Parquet)
- return Tuple3.of(null, null, valueCount);
+ // Statistics are missing for this column (common after expanding
+ // hoodie.metadata.index.column.stats.column.list, async clustering,
etc.).
+ // Do NOT treat missing stats as "all values are null" — that causes
+ // silent data loss. Use nullCnt = -1 as an explicit "unknown"
sentinel.
+ // ExpressionEvaluators will keep the file when stats are unknown.
+ return Tuple3.of(null, null, -1L);
Review Comment:
🤖 `ExpressionEvaluators.Not` just returns `!child.eval(...)`, so now that
leaf evaluators return `true` for unknown stats, a filter like `newcol NOT IN
(1, 2)` or `NOT (newcol = 5)` prunes every old file without stats for `newcol`;
before this change those files were kept, because the leaf returned `false` on
null min/max. Could you make `Not` keep the file when any column it references
has unknown stats, and add a test for the `NOT` case?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/source/ExpressionEvaluators.java:
##########
@@ -237,6 +237,9 @@ public static EqualTo getInstance() {
@Override
protected boolean eval(@Nonnull Object val, ColumnStats columnStats,
LogicalType type) {
+ if (columnStats.isUnknown()) {
+ return true; // cannot prove mismatch → keep the file
Review Comment:
🤖 nit: the `isUnknown()` check is repeated in every evaluator, so would it
make sense to handle it once in the shared base `eval` path (and drop the
inline comment that only appears here)?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
