Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: don't re-write CASTs to 'list's with the :: operator #31696

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ParkMyCar
Copy link
Member

This PR changes our SQL rendering to use the standard SQL CAST expression when a cast involves the list type, instead of rewriting the expression to use the Postgres :: operator.

-- For example consider the following expression:
CAST(<field> AS <type> list)[index]
-- Today get's re-written as:
(<field>)::<type> list[index]

-- these two expressions are not the same!

The rewrite to the Postgres operator was originally introduced in #3149

I went back and forth on a number of different solutions to fix this panic, and what's in this PR is sort of the least bad. My major gripe with this implementation is adding the AstDataType trait, it feels like it breaks the abstraction of AstInfo::DataType, but maybe this isn't too bad?

Other changes I tried:

  1. Parsing int list[] as indexing into a list as opposed to the type Array<List<int>>. We already don't support arrays of lists, but this behavior felt like a very big change.
  2. Supporting wrapping parens around the type, e.g. (int list). Also not a bad solution but the parser impl wasn't too straight forward, and given the root cause was re-writing the CAST statements, this felt like a bit of a hack?
  3. Wrapping the entire :: expression in parens, e.g. ((<field>)::<type> list)[index]. But this also changes the semantics a bit because now the expression provided to the subscript operator is a nested expression. Concretely going from text -> AST -> text -> AST failed to roundtrip AST.

Motivation

A customer hit this panic in their environment.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@ParkMyCar ParkMyCar requested a review from a team as a code owner February 27, 2025 23:07
@ParkMyCar ParkMyCar requested a review from aljoscha February 27, 2025 23:07
@ParkMyCar ParkMyCar added the lts-backport-v25.1 Needs to be backported into the v25.1 LTS release label Feb 27, 2025
@ParkMyCar ParkMyCar force-pushed the parser/parse-as-index-into-list branch from 08ede96 to 1932e3b Compare February 27, 2025 23:07
@ParkMyCar ParkMyCar requested a review from ggevay February 27, 2025 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lts-backport-v25.1 Needs to be backported into the v25.1 LTS release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant