Skip to content

fix(testing): Check both type and data for GenericLiteral in verifier#27108

Merged
hantangwangd merged 1 commit intoprestodb:masterfrom
hantangwangd:check_data_type_for_generic_literal
Feb 11, 2026
Merged

fix(testing): Check both type and data for GenericLiteral in verifier#27108
hantangwangd merged 1 commit intoprestodb:masterfrom
hantangwangd:check_data_type_for_generic_literal

Conversation

@hantangwangd
Copy link
Copy Markdown
Member

@hantangwangd hantangwangd commented Feb 9, 2026

Description

Currently, when UnaliasSymbolReferences performs mapping and canonicalization to compare expression equality, it validates both the type and value for a ConstantExpression. However, in test framework, RowExpressionVerifier does not check the type when verifying if an expression equals a GenericLiteral—it validates only the value. This inconsistency can cause issues, as demonstrated by the following test case:

        assertPlan("select nan(), infinity(), cast (nan() as real), cast(infinity() as real)",
                anyTree(strictProject(
                        ImmutableMap.of(
                                "col_1", expression("double 'NaN'"),
                                "col_2", expression("double 'Infinity'"),
                                "col_3", expression("real 'NaN'"),
                                "col_4", expression("real 'Infinity'")),
                        values())));

The test fails with the exception:

java.lang.IllegalStateException: Ambiguous expression double 'NaN' matches multiple assignments [NaN, 2143289344]

This PR fixes the issue by ensuring RowExpressionVerifier checks both the type and data for a GenericLiteral, aligning its equality validation with the behavior in the core path.

Motivation and Context

Fix the issues caused by inconsistency in constant expression equality checking between the test framework and the core path.

Impact

N/A

Test Plan

Newly added test cases in TestLogicalPlanner to verify the scenario in the PR description

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.
  • If adding new dependencies, verified they have an OpenSSF Scorecard score of 5.0 or higher (or obtained explicit TSC approval for lower scores).

Release Notes

== NO RELEASE NOTE ==

Summary by Sourcery

Align expression verification for generic literals with planner behavior and add coverage for NaN/Infinity and integer/bigint constants.

Bug Fixes:

  • Ensure RowExpressionVerifier compares both type and value when matching GenericLiteral expressions to avoid ambiguous matches.

Tests:

  • Add logical planner tests covering NaN/Infinity across double and real types.
  • Add logical planner test covering bigint vs integer literals in projections.
  • Adjust spatial join and rewrite-row-expressions tests to use typed bigint literals for numeric constants.

@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai bot commented Feb 9, 2026

Reviewer's Guide

Align RowExpressionVerifier’s GenericLiteral handling with core constant expression equality semantics by adding a type check alongside value comparison and updating/adding planner tests to use explicitly typed literals (including NaN/Infinity and bigint/integer cases).

File-Level Changes

Change Details Files
Ensure row expression verification for GenericLiteral matches on both type and value to avoid ambiguous constant matches across numeric types.
  • Update GenericLiteral visitor in RowExpressionVerifier to short-circuit and return false when the expected literal SQL type does not match the actual RowExpression base type signature.
  • Retain existing literal data comparison via compareLiteral once type compatibility is confirmed.
presto-main-base/src/test/java/com/facebook/presto/sql/planner/assertions/RowExpressionVerifier.java
Add logical planner regression tests to cover NaN/Infinity and integer/bigint cast scenarios using typed GenericLiterals.
  • Introduce testInfinityAndNaNExpression asserting the plan contains distinct double and real NaN/Infinity typed literals in a strict project.
  • Introduce testBigintAndIntegerExpression asserting separate integer and bigint typed literals for the same numeric value in the plan.
presto-main-base/src/test/java/com/facebook/presto/sql/planner/TestLogicalPlanner.java
Adjust existing tests that build RowExpressions from SQL strings to use explicitly typed bigint literals, keeping them compatible with stricter GenericLiteral type checks.
  • Change spatial join filter test to use "ST_Distance(a, b) < BIGINT '5000'" so the comparison literal has an explicit bigint type.
  • Modify constant arithmetic rewrite test to use "bigint '1' + 2" as the projected expression, ensuring the left operand’s type matches the expected BIGINT variable.
presto-main-base/src/test/java/com/facebook/presto/geospatial/TestExtractSpatialInnerJoin.java
presto-main-base/src/test/java/com/facebook/presto/sql/planner/iterative/rule/TestRewriteRowExpressions.java

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@hantangwangd hantangwangd force-pushed the check_data_type_for_generic_literal branch from 0ac358e to a8dcbff Compare February 10, 2026 18:49
@hantangwangd hantangwangd marked this pull request as ready for review February 11, 2026 00:53
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@hantangwangd hantangwangd merged commit aabe558 into prestodb:master Feb 11, 2026
115 of 118 checks passed
@hantangwangd hantangwangd deleted the check_data_type_for_generic_literal branch February 11, 2026 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants