[fix](planner) align legacy literal compareLiteral with Nereids ComparableLiteral semantics#63481
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…rableLiteral semantics IPv4Literal/IPv6Literal.compareLiteral both used to return 0 unconditionally, making any two IP literals appear equal to LiteralExpr.equals(). The downstream effect: WHERE ip4 != '1.1.1.1' AND ip4 != '1.1.1.2' collapsed to just the first conjunct during Set<Expr> dedup inside the legacy planner's ScanNode.expressionToRanges / PartitionColumnFilter / HashDistributionPruner paths. Same shape for NOT BETWEEN and NOT IN. Multiple rows that should have been filtered out leaked through, and EXPLAIN only showed the first predicate. Apache Doris issue: apache#62672 SelectDB Jira: CIR-20160 Fix: - IPv4Literal: compare on the long value; throw on cross-type. Override equals/hashCode so LiteralExpr.equals stops short-circuiting to the now-throwing compareLiteral on non-IPv4 peers. - IPv6Literal: canonicalize via InetAddress.getByName so "::1" and "0:0:0:0:0:0:0:1" hash and compare equal, then 128-bit unsigned compare via BigInteger; same equals/hashCode pattern. While auditing the rest of the legacy literals against their Nereids counterparts' ComparableLiteral implementations, three more had the same return-0 shape but happen to be unreachable from SQL today (MAP/STRUCT column comparison predicates are rejected at analyze time; TIME is not allowed as an OLAP column type). Switch them to throw so the bug surfaces loudly if a future planner change ever reaches them: - MapLiteral.compareLiteral throws - StructLiteral.compareLiteral throws - TimeV2Literal.compareLiteral throws (+ equals/hashCode on getValue() so dedup paths that don't go through compareLiteral still work) Tests: - regression-test/suites/query_p0/sql_functions/ip_functions/ test_ipv4_ipv6_multi_not_equal.groovy covers IPv4/IPv6 multiple !=, NOT BETWEEN, NOT IN + != combos, with EXPLAIN assertions proving every conjunct survives. - fe-catalog unit tests (new) cover compareLiteral + equals for every legacy LiteralExpr subclass that survives in the legacy planner (98 tests total, all green via `mvn -pl fe-catalog -am test`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pin equals() / hashCode() semantics for every legacy Expr subclass that overrides equals — covering ArithmeticExpr, BinaryPredicate, CastExpr, CompoundPredicate, FunctionCallExpr, InformationFunction, InPredicate, IsNullPredicate, LikePredicate, MatchPredicate, SlotRef, VariableExpr, CaseExpr. 33 cases total. Each subclass test asserts: - same children + same distinguishing field (op / isNot* / name / ...) → equal - different distinguishing field → not equal - different children → not equal - (where applicable) hashCode consistency Skipped: - BetweenPredicate / SearchPredicate: no usable public constructor for unit construction (parser-only paths) - ColumnRefExpr / EncryptKeyRef / LambdaFunctionExpr / TimestampArithmeticExpr: no equals override, inherit Expr.equals default which is already exercised through children comparison in the covered subclass tests Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
csun5285
force-pushed
the
fix/ipv4-ipv6-multi-not-equal
branch
from
May 21, 2026 10:26
bd2b2cb to
9bac9d6
Compare
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31102 ms |
Contributor
TPC-DS: Total hot run time: 168949 ms |
Contributor
|
run buildall |
Contributor
TPC-H: Total hot run time: 31204 ms |
Contributor
TPC-DS: Total hot run time: 169634 ms |
morrySnow
previously approved these changes
May 25, 2026
Contributor
|
/review |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary: I found one blocking correctness issue in the new IPv6 literal comparison implementation. The PR goal is clear and the IPv4 path appears to address the predicate-dedup bug, but IPv6 comparison is still incorrect for IPv4-mapped IPv6 literals that the class already accepts.
Critical checkpoints: goal/test coverage: the PR targets legacy literal equality/comparison for predicate dedup and adds unit/regression coverage, but mapped IPv6 forms are untested and currently wrong. Scope: the change is focused. Concurrency/lifecycle/config/compatibility/persistence/data-write/observability: not applicable for these FE literal/test changes. Parallel paths: IPv4 and IPv6 were both updated, but IPv6 needs mapped-address handling consistent with its accepted syntax. Error handling: no ignored checked status; however the new helper converts a valid literal into the wrong numeric domain rather than failing. Tests: new tests cover compact vs expanded IPv6 and multi-predicate SQL, but miss IPv4-mapped IPv6, which is the failing edge case. User focus: no additional user-provided review focus was supplied.
…n legacy compare/equals InetAddress.getByName collapsed mapped forms like ::ffff:0.0.0.1 (and ::ffff:0:1) into a 4-byte Inet4Address, so toUnsigned128 produced BigInteger(1) — the same value as ::1 — and predicate dedup folded the two together. Switch the legacy IPv6Literal to com.googlecode.ipv6 IPv6Address (same library the Nereids IPv6Literal already uses), which always materializes 128 bits and gives consistent compare/equals/hashCode across both planners. Adds ::ffff:0.0.0.1 vs ::1 coverage in both LiteralExprEqualsTest and LiteralExprCompareLiteralTest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
removed
the
approved
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 32051 ms |
Contributor
TPC-DS: Total hot run time: 173919 ms |
Contributor
|
/review |
Member
|
/review |
Contributor
|
OpenCode automated review failed and did not complete. Error: Error: The user rejected permission to use this specific tool call. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
|
/review Review过程中严禁尝试访问任何代码库以外的内容或在本机全局搜索任何内容。 |
Contributor
There was a problem hiding this comment.
Automated review summary:
No additional blocking issues found in the current head beyond the already-known review context. The checked-out code appears to have addressed the existing IPv6 mapped-address parsing concern, the TIMEV2 equals/hashCode mismatch, and the regression-test final drop-table concern.
Critical checkpoint conclusions:
- Goal and tests: the PR targets incorrect legacy Expr/LiteralExpr equality and literal comparison behavior used by predicate deduplication and pruning. The implementation and added FE unit tests/regression case are aligned with that goal.
- Scope: the changes are focused on legacy literal comparison/equality plus targeted tests; no broad unrelated behavior changes found.
- Concurrency: no new shared mutable state, locking, thread, or async behavior introduced.
- Lifecycle/static initialization: no non-trivial lifecycle or cross-TU/static initialization issue found.
- Configuration: no new configuration items.
- Compatibility/protocol/storage: no FE-BE protocol, persistence, storage format, transaction, or rolling-upgrade compatibility changes found.
- Parallel paths: legacy IPv6 canonicalization now matches the existing Nereids IPv6Address-based path; no other directly parallel path needing the same fix was found.
- Conditional/error handling: unsupported MAP/STRUCT/TIMEV2 compareLiteral paths now fail loudly instead of returning equality, which is safer than silent deduplication.
- Test coverage: FE unit tests and a deterministic regression test cover the main corrected scenarios, including IPv4/IPv6 multi-predicate dedup, IPv6 canonical forms, IPv4-mapped IPv6, and TIMEV2 scale hash consistency.
- Test execution: attempted
mvn -pl fe-catalog -Dtest=ExprEqualsTest,LiteralExprCompareLiteralTest,LiteralExprEqualsTest test -Dskip.doc=true, which failed because sibling SNAPSHOT modules were not installed. Retried with-am -DfailIfNoTests=false, but the reactor stopped at fe-thrift becausethirdparty/installed/bin/thriftis missing in this runner. - Observability/performance: no new observability required; repeated IPv6Address parsing is in planner literal comparison/equality and not a BE data hot path.
User focus: review operations were confined to the repository checkout; I did not intentionally inspect code or files outside the workspace.
CalvinKirs
reviewed
May 26, 2026
CalvinKirs
approved these changes
May 26, 2026
morrySnow
approved these changes
May 26, 2026
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
May 29, 2026
…ereids ComparableLiteral semantics (apache#63481) IPv4Literal/IPv6Literal.compareLiteral both used to return 0 unconditionally, making any two IP literals appear equal to LiteralExpr.equals(). The downstream effect: WHERE ip4 != '1.1.1.1' AND ip4 != '1.1.1.2' collapsed to just the first conjunct during Set<Expr> dedup inside the legacy planner's ScanNode.expressionToRanges / PartitionColumnFilter / HashDistributionPruner paths. Same shape for NOT BETWEEN and NOT IN. Multiple rows that should have been filtered out leaked through, and EXPLAIN only showed the first predicate. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
May 29, 2026
…ereids ComparableLiteral semantics (apache#63481) IPv4Literal/IPv6Literal.compareLiteral both used to return 0 unconditionally, making any two IP literals appear equal to LiteralExpr.equals(). The downstream effect: WHERE ip4 != '1.1.1.1' AND ip4 != '1.1.1.2' collapsed to just the first conjunct during Set<Expr> dedup inside the legacy planner's ScanNode.expressionToRanges / PartitionColumnFilter / HashDistributionPruner paths. Same shape for NOT BETWEEN and NOT IN. Multiple rows that should have been filtered out leaked through, and EXPLAIN only showed the first predicate. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 1, 2026
zhaorongsheng
pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Jun 4, 2026
…rableLiteral semantics (apache#63481) IPv4Literal/IPv6Literal.compareLiteral both used to return 0 unconditionally, making any two IP literals appear equal to LiteralExpr.equals(). The downstream effect: WHERE ip4 != '1.1.1.1' AND ip4 != '1.1.1.2' collapsed to just the first conjunct during Set<Expr> dedup inside the legacy planner's ScanNode.expressionToRanges / PartitionColumnFilter / HashDistributionPruner paths. Same shape for NOT BETWEEN and NOT IN. Multiple rows that should have been filtered out leaked through, and EXPLAIN only showed the first predicate. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
morningman
pushed a commit
to csun5285/doris
that referenced
this pull request
Jun 24, 2026
…ereids ComparableLiteral semantics (apache#63481) IPv4Literal/IPv6Literal.compareLiteral both used to return 0 unconditionally, making any two IP literals appear equal to LiteralExpr.equals(). The downstream effect: WHERE ip4 != '1.1.1.1' AND ip4 != '1.1.1.2' collapsed to just the first conjunct during Set<Expr> dedup inside the legacy planner's ScanNode.expressionToRanges / PartitionColumnFilter / HashDistributionPruner paths. Same shape for NOT BETWEEN and NOT IN. Multiple rows that should have been filtered out leaked through, and EXPLAIN only showed the first predicate. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IPv4Literal/IPv6Literal.compareLiteral both used to return 0 unconditionally, making any two IP literals appear equal to LiteralExpr.equals(). The downstream effect:
WHERE ip4 != '1.1.1.1' AND ip4 != '1.1.1.2'
collapsed to just the first conjunct during Set dedup inside the legacy planner's ScanNode.expressionToRanges / PartitionColumnFilter / HashDistributionPruner paths. Same shape for NOT BETWEEN and NOT IN. Multiple rows that should have been filtered out leaked through, and EXPLAIN only showed the first predicate.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)