fix(native): Fix Dereference expression index type in VeloxToPrestoExprConverter

[pramodsatya]

Description

Changes the type of index in DEREFERENCE expression from BIGINT to INTEGER to match the Presto RowExpression type.

Motivation and Context

Resolves #27037.

Impact

Sql invoked functions with dereference expression won't fail with native expression optimizer enabled.

Test Plan

Added e2e tests.

== NO RELEASE NOTE ==

Summary by Sourcery

Align DEREFERENCE expression index type with Presto RowExpression and add coverage for native optimizer queries using dereference.

Bug Fixes:

• Change DEREFERENCE expression index type from BIGINT to INTEGER in VeloxToPrestoExprConverter to match Presto RowExpression expectations and avoid failures in SQL-invoked functions when the native optimizer is enabled.

Tests:

• Extend native optimizer end-to-end tests to cover dereference expressions via array_least_frequent usage.

[sourcery-ai]

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adjusts the Velox-to-Presto dereference expression index type to use INTEGER instead of BIGINT and adds end-to-end tests to cover dereference usage via array_least_frequent under the native optimizer.

Sequence diagram for DEREFERENCE conversion with INTEGER index

sequenceDiagram
  participant NativeOptimizer
  participant VeloxToPrestoExprConverter as Converter
  participant velox_core_FieldAccessTypedExpr as FieldAccessExpr
  participant velox_core_ConstantTypedExpr as ConstantExpr
  participant protocol_RowExpression as RowExpr

  NativeOptimizer->>Converter: getDereferenceExpression(FieldAccessExpr)
  Converter->>FieldAccessExpr: inputs()
  FieldAccessExpr-->>Converter: std_vector_TypedExprPtr
  Converter->>FieldAccessExpr: index()
  FieldAccessExpr-->>Converter: index_int32
  Converter->>ConstantExpr: new ConstantTypedExpr(velox_INTEGER, index_int32)
  Converter->>Converter: build dereferenceInputs[0] = base_expr
  Converter->>Converter: build dereferenceInputs[1] = ConstantExpr
  loop for each input in dereferenceInputs
    Converter->>Converter: getRowExpression(input)
    Converter-->>Converter: RowExpr
  end
  Converter-->>NativeOptimizer: RowExpr representing DEREFERENCE with INTEGER index

Loading

Class diagram for updated dereference conversion in VeloxToPrestoExprConverter

classDiagram
  class VeloxToPrestoExprConverter {
    +getDereferenceExpression(dereferenceExpr : velox_core_FieldAccessTypedExprPtr) SpecialFormExpressionPtr
    +getRowExpression(input : velox_core_TypedExprPtr) protocol_RowExpressionPtr
  }

  class velox_core_FieldAccessTypedExpr {
    +inputs() std_vector_velox_core_TypedExprPtr
    +index() int32_t
  }

  class velox_core_TypedExpr {
  }

  class velox_core_ConstantTypedExpr {
    +ConstantTypedExpr(type : velox_TypePtr, value : int32_t)
  }

  class velox_Type {
  }

  class protocol_RowExpression {
  }

  VeloxToPrestoExprConverter --> velox_core_FieldAccessTypedExpr : uses
  VeloxToPrestoExprConverter --> velox_core_TypedExpr : builds_dereferenceInputs
  VeloxToPrestoExprConverter --> velox_core_ConstantTypedExpr : creates_index_constant
  velox_core_ConstantTypedExpr --> velox_Type : has_type
  VeloxToPrestoExprConverter --> protocol_RowExpression : returns

Loading

File-Level Changes

Change	Details	Files
Align dereference expression index type with Presto RowExpression expectations.	Change the constant index expression type from BIGINT to INTEGER when building dereference expressions in VeloxToPrestoExprConverter, including narrowing the cast from int64_t to int32_t. Ensure the serialized JSON RowExpression for dereference uses an INTEGER index to match Presto’s type system.	presto-native-execution/presto_cpp/main/types/VeloxToPrestoExpr.cpp
Add native optimizer e2e coverage for dereference expressions used by array_least_frequent.	Extend TestNativeSidecarPlugin native optimizer tests to run queries invoking array_least_frequent over a row field (orders_ex.quantities). Add a second array_least_frequent query using split(comment, '') with an integer argument to exercise dereference behavior under different input shapes.	presto-native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#27037	Fix the native expression optimizer crash when running queries using array_least_frequent (e.g., on orders_ex.quantities and split(comment, '') from nation) by correcting the handling of the relevant expressions.	✅
#27037	Add regression tests to ensure queries using array_least_frequent with native expression optimizer enabled execute successfully without crashing.	✅

Possibly linked issues

• native] Native expression optimizer crashes worker while optimizing array_least_frequent #27037: PR corrects dereference index type in native optimizer, addressing the optimizer failure described in the issue.

---

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

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've left some high level feedback:

• The change from BIGINT to INTEGER introduces a narrowing cast on dereferenceExpr->index(); consider adding an assertion or explicit range check (or a comment explaining why the index is guaranteed to fit in int32) to make the assumption about index bounds clear.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The change from BIGINT to INTEGER introduces a narrowing cast on `dereferenceExpr->index()`; consider adding an assertion or explicit range check (or a comment explaining why the index is guaranteed to fit in int32) to make the assumption about index bounds clear.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[pramodsatya]

@aditi-pandit, could you please help review this fix?

[pdabre12]

@pramodsatya Changes look good.
Seems like the test case is failing because of a known varchar(N) issue, can you instead add another test case without varchar column ?
Optionally, you can keep this test case with an assertQueryFails message , whatever you prefer.

[aditi-pandit]

Thanks @pramodsatya. Please add more tests.

[Copilot]

Pull request overview

This PR fixes a critical bug where the native expression optimizer crashes when processing DEREFERENCE expressions. The fix changes the index type from BIGINT to INTEGER to match Presto's RowExpression protocol expectations.

Changes:

• Updated DEREFERENCE expression index type from BIGINT/int64_t to INTEGER/int32_t in VeloxToPrestoExprConverter
• Added comprehensive end-to-end tests for dereference expressions including nested row access patterns
• Added regression tests for array_least_frequent function which triggered the original crash

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
presto-native-execution/presto_cpp/main/types/VeloxToPrestoExpr.cpp	Fixed DEREFERENCE expression index type from BIGINT to INTEGER and cast from int64_t to int32_t to match Presto protocol
presto-native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java	Added e2e tests for dereference expressions with nested rows and array_least_frequent function

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[pramodsatya]

Thanks @aditi-pandit, @pdabre12, added more tests, could you please take another look?

[aditi-pandit]

Thanks @pramodsatya

[pdabre12]

LGTM.