Uh oh!
There was an error while loading. Please reload this page.
[fix](planner) align legacy literal compareLiteral with Nereids ComparableLiteral semantics - #63481
Merged
Merged
Conversation
hello-stephen
commented
May 21, 2026
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>
csun5285force-pushed
the
fix/ipv4-ipv6-multi-not-equal
branch
from
May 21, 2026 10:26
bd2b2cb to
9bac9d6Comparecsun5285
commented
May 21, 2026
ContributorAuthor
run buildall |
1 similar comment
csun5285
commented
May 22, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
May 22, 2026
Contributor
TPC-H: Total hot run time: 31102 ms |
hello-stephen
commented
May 22, 2026
Contributor
TPC-DS: Total hot run time: 168949 ms |
yiguolei
commented
May 22, 2026
Contributor
run buildall |
hello-stephen
commented
May 22, 2026
Contributor
TPC-H: Total hot run time: 31204 ms |
hello-stephen
commented
May 22, 2026
Contributor
TPC-DS: Total hot run time: 169634 ms |
morrySnow
previously approved these changes
May 25, 2026
morrySnow
commented
May 25, 2026
Contributor
/review |
Contributor
PR approved by at least one committer and no changes requested. |
Contributor
PR approved by anyone and no changes requested. |
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.
Uh oh!
There was an error while loading. Please reload this page.
…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>
csun5285
commented
May 25, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
May 25, 2026
Contributor
TPC-H: Total hot run time: 32051 ms |
hello-stephen
commented
May 25, 2026
Contributor
TPC-DS: Total hot run time: 173919 ms |
morrySnow
commented
May 25, 2026
Contributor
/review |
eldenmoon
commented
May 26, 2026
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. |
zclllyybb
commented
May 26, 2026
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.
Uh oh!
There was an error while loading. Please reload this page.
CalvinKirs
approved these changes
May 26, 2026
morrySnow
approved these changes
May 26, 2026
Uh oh!
There was an error while loading. Please reload this page.
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>
Closed
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>
Closed
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 freeto 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)