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

fix ColumnType to RelDataType conversion for nested arrays #16138

Conversation

clintropolis
Copy link
Member

@clintropolis clintropolis commented Mar 15, 2024

Description

RowSignatures.toRelDataType did not correctly handle conversion of nested array types such as ARRAY<ARRAY<LONG>> etc, since the ARRAY handling case only checked for the primitive column types. I have modified this code to split out the ColumnType to RelDataType conversion into a new method, RowSignatures. columnTypeToRelDataType, and handle arrays by recursively calling this method to convert the element type to a RelDataType so that it can handle any type of Druid array.

I kind of think this method maybe should be moved into Calcites since a bunch of the other conversion methods that don't involve RowSignature live there, but I haven't moved it yet...

Only the second added test would fail, despite just being a count(*) wrapper around the first one. Apparently this query used to run fine, but I believe the changes in #16033 which resulted in RowSignatures.toRelDataType started exercising this path a bit more frequently, which is how the problem was noticed.


This PR has:

  • been self-reviewed.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

@gianm gianm merged commit 5afd5c4 into apache:master Mar 19, 2024
83 checks passed
@clintropolis clintropolis deleted the fix-column-type-to-rel-data-type-conversion-nested-arrays branch March 19, 2024 07:13
@adarshsanjeev adarshsanjeev added this to the 30.0.0 milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants