[jira] [Commented] (IMPALA-15421) Iceberg predicate subsetting drops a needed conjunct when two residuals sanitize to the same string

["ASF subversion and git services (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/IMPALA-15421?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18120863#comment-18120863 ] 

ASF subversion and git services commented on IMPALA-15421:
----------------------------------------------------------

Commit 9297acd1bb0abeadff268cda0d2d551b12c48931 in impala's branch refs/heads/master from Zoltan Borok-Nagy
[ https://gitbox.apache.org/repos/asf?p=impala.git;h=9297acd1b ]

IMPALA-15421: Fix dropped conjuncts in Iceberg predicate subsetting

IcebergScanPlanner collected the residual expressions of the
FileScanTasks into a TreeSet ordered by
ExpressionUtil.toSanitizedString(). The sanitized string replaces
literal values with placeholders, e.g. both 'id != 11' and
'id != 21' become 'id != (2-digit-int)', so the set only kept one of
two different residuals. With ICEBERG_PREDICATE_PUSHDOWN_SUBSETTING
(on by default) the conjunct of the lost residual was then skipped
from the scan node, and the query returned rows that should have been
filtered out. This happens when non-identity partition transforms
(e.g. truncate or day) leave different residuals for different files.

This patch keys the residuals by Expression.toString(), which prints
the literals in full. It uses a TreeMap so the order of the retained
conjuncts stays deterministic. Consecutive files often have the same
residual object (e.g. every file of an unpartitioned table), so the
string conversion is skipped for them. This also removes the cost of
sanitizing the residual of every file during planning. A predicate
that is part of multiple residuals is only retained once.

Testing:
 - Added EE tests with truncate and day partition transforms. Both
   returned wrong results without the fix.
 - Ran PlannerTest and test_iceberg.py.

Change-Id: Ic2b0ef09255ead578ab4328885002819dddee013
Assisted-by: Claude Opus 5.5 (1M context) <no...@anthropic.com>
Reviewed-on: http://gerrit.cloudera.org:8080/24947
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>


> Iceberg predicate subsetting drops a needed conjunct when two residuals sanitize to the same string
> ---------------------------------------------------------------------------------------------------
>
>                 Key: IMPALA-15421
>                 URL: https://issues.apache.org/jira/browse/IMPALA-15421
>             Project: IMPALA
>          Issue Type: Bug
>          Components: Frontend
>            Reporter: Zoltán Borók-Nagy
>            Assignee: Zoltán Borók-Nagy
>            Priority: Critical
>              Labels: correctness, impala-iceberg, impala-iceberg-active-backlog
>
> IcebergScanPlanner collects the residual expression of every FileScanTask into a set
> that uses Iceberg's sanitized string as the comparator:
> {code:java}
> // IcebergScanPlanner.java:125
> private final Set<Expression> residualExpressions_ =
>     new TreeSet<>(Comparator.comparing(ExpressionUtil::toSanitizedString));
> {code}
> {{toSanitizedString()}} replaces numeric literals with placeholders, so different
> residuals compare as equal:
> {noformat}
> id != 12345  ->  id != (5-digit-int)
> id != 54321  ->  id != (5-digit-int)
> {noformat}
> The set keeps only the first one. With ICEBERG_PREDICATE_PUSHDOWN_SUBSETTING=true (the
> default), trySubsettingPredicatesBeingPushedDown() retains only the Impala conjuncts that
> map to residuals in the set. The other conjunct goes to {{skippedExpressions_}} and the
> scanner never evaluates it, so rows that should be filtered are returned.
> This happens when different files have different residuals that sanitize to the same
> string. Non-identity partition transforms (truncate, bucket) are a typical cause. For
> {{TRUNCATE(10000, id)}} and {{id != 12345 AND id != 54321}}, Iceberg's ResidualEvaluator
> returns {{id != 12345}} for partition 10000 and {{id != 54321}} for partition 50000.
> String literals are hashed by the sanitizer, so they don't collide.
> h3. Repro
> {code:sql}
> CREATE TABLE ice_residual (id INT)
>   PARTITIONED BY SPEC (TRUNCATE(10000, id)) STORED AS ICEBERG;
> INSERT INTO ice_residual VALUES (12345), (12346), (54321), (54322);
> SELECT id FROM ice_residual WHERE id != 12345 AND id != 54321 ORDER BY id;
> {code}
> Expected: 12346, 54322.
> Actual (on master a6af29eee0):
> {noformat}
> +-------+
> | id    |
> +-------+
> | 12346 |
> | 54321 |
> | 54322 |
> +-------+
> {noformat}
> The EXPLAIN output shows that the second conjunct is no longer evaluated by the scan:
> {noformat}
> 00:SCAN HDFS [ice_residual_repro.ice_residual, RANDOM]
>    HDFS partitions=2/2 files=2 size=716B
>    predicates: id != CAST(12345 AS INT)
>    skipped Iceberg predicates: id != CAST(54321 AS INT)
> {noformat}
> Which of the two values leaks depends on which file planFiles() returns first. With
> ICEBERG_PREDICATE_PUSHDOWN_SUBSETTING=false, the same query returns the correct 12346,
> 54322.
> Workaround: SET ICEBERG_PREDICATE_PUSHDOWN_SUBSETTING=false.
> h3. Proposed fix
> Key the collection by the exact {{Expression.toString()}}, which prints literals in full.
> Use a TreeMap<String, Expression> so the order of the retained conjuncts stays
> deterministic for planner tests. Add an identity short-circuit for the common case where
> every file returns the same residual object (e.g. unpartitioned tables). This also
> removes the per-file sanitization cost from planning, about 2 us per file on
> unpartitioned tables in a 100k-file microbenchmark. Add a regression test for the case
> above.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-all-unsubscribe@impala.apache.org
For additional commands, e-mail: issues-all-help@impala.apache.org