Skip to content

Bug fix for array type selector causing array aggregation over window frame fail#16653

Merged
kgyrtkirk merged 7 commits intoapache:masterfrom
sreemanamala:win-class-cast
Jul 17, 2024
Merged

Bug fix for array type selector causing array aggregation over window frame fail#16653
kgyrtkirk merged 7 commits intoapache:masterfrom
sreemanamala:win-class-cast

Conversation

@sreemanamala
Copy link
Copy Markdown
Contributor

@sreemanamala sreemanamala commented Jun 25, 2024 •
edited
Loading

Description

This commit fixes the windowed aggregation involving array type selector.

example query (earlier used to fail with a defensive exception):

select countryName, cityName, channel, array_concat_agg(ARRAY['N/A', channel], 10000) over (partition by cityName order by countryName) as c
from wikipedia
where countryName in ('Austria', 'Republic of Korea') and (cityName in ('Horsching', 'Vienna', 'Seoul', 'Jeonju') or cityName is null)
group by countryName, cityName, channel
Key changed/added classes in this PR
  • DefaultColumnSelectorFactoryMaker
  • ExpressionSelectors

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

kgyrtkirk
kgyrtkirk reviewed Jun 25, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java Outdated
kgyrtkirk
kgyrtkirk approved these changes Jul 8, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@kgyrtkirk kgyrtkirk left a comment

Choose a reason for hiding this comment

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

LGTM
@clintropolis: I think this will be good - however this is close to null / [] / [null] stuff ... do you have any notices?

clintropolis
clintropolis reviewed Jul 9, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java Outdated
if (val != null) {
NonnullPair<ExpressionType, Object[]> coerced = ExprEval.coerceListToArray(
Arrays.asList((Object[]) val),
homogenizeMultiValue
Copy link
Copy Markdown
Member

@clintropolis clintropolis Jul 9, 2024

Choose a reason for hiding this comment

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

wait, actual arrays should never be coerced, this is strictly for multi-value strings, why do we need to coerce here?

Copy link
Copy Markdown
Contributor Author

@sreemanamala sreemanamala Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @clintropolis, Understood. changed it to pass through directly for Object arrays. I checked on few scenarios and they worked fine. Would this not create any problem if the underlying Object is not String or Numeric? I tried Array< Array< String >> and that worked.

clintropolis
clintropolis approved these changes Jul 17, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/segment/virtual/ExpressionSelectors.java
final Class<?> clazz = selector.classOfObject();
if (Number.class.isAssignableFrom(clazz) || String.class.isAssignableFrom(clazz)) {
// Number, String supported as-is.
if (Number.class.isAssignableFrom(clazz) || String.class.isAssignableFrom(clazz) || Object[].class.isAssignableFrom(clazz)) {
Copy link
Copy Markdown
Member

@clintropolis clintropolis Jul 17, 2024

Choose a reason for hiding this comment

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

i suppose this would have also ended up in the 'else' case, but this seems fine too

@kgyrtkirk kgyrtkirk merged commit 40ef9fc into apache:master Jul 17, 2024
@sreemanamala sreemanamala deleted the win-class-cast branch July 17, 2024 12:10
edgar2020 pushed a commit to edgar2020/druid that referenced this pull request Jul 19, 2024
@sreemanamala @edgar2020
Bug fix for array type selector causing array aggregation over window…
82e6f86 
… frame fail (apache#16653)
edgar2020 pushed a commit to edgar2020/druid that referenced this pull request Jul 19, 2024
@sreemanamala @edgar2020
Bug fix for array type selector causing array aggregation over window…
17e6936 
… frame fail (apache#16653)
sreemanamala added a commit to sreemanamala/druid that referenced this pull request Aug 6, 2024
@sreemanamala
Bug fix for array type selector causing array aggregation over window…
dbe8ae1 
… frame fail (apache#16653)
@kfaraz kfaraz added this to the 31.0.0 milestone Oct 4, 2024
@kfaraz kfaraz mentioned this pull request Oct 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clintropolis clintropolis clintropolis approved these changes

@kgyrtkirk kgyrtkirk kgyrtkirk approved these changes

Assignees

No one assigned

Labels

Area - Querying Area - Segment Format and Ser/De

Projects

None yet

Milestone

31.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@sreemanamala @clintropolis @kgyrtkirk @kfaraz