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.

Sign up for GitHub

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

Jump to bottom

Add serde for ColumnBasedRowsAndColumns to fix window queries without group by #16658

Merged
merged 29 commits into from
Sep 17, 2024
Merged

Add serde for ColumnBasedRowsAndColumns to fix window queries without group by #16658

merged 29 commits into from
Sep 17, 2024

Conversation

sreemanamala
Copy link
Contributor

@sreemanamala sreemanamala commented Jun 26, 2024 •
edited
Loading

Description

Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine.

Key changed/added classes in this PR
  • RowsAndColumns
  • FrameRowsAndColumns
  • DruidDefaultSerializersModule

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.

@sreemanamala sreemanamala marked this pull request as draft June 26, 2024 05:37
@sreemanamala sreemanamala marked this pull request as ready for review June 26, 2024 15:00
@sreemanamala sreemanamala reopened this Jul 22, 2024
@sreemanamala sreemanamala requested a review from kgyrtkirk July 22, 2024 04:59
@sreemanamala sreemanamala marked this pull request as draft July 22, 2024 04:59
@sreemanamala sreemanamala marked this pull request as ready for review July 25, 2024 05:27
kgyrtkirk
kgyrtkirk reviewed Sep 5, 2024
View reviewed changes
processing/src/main/java/org/apache/druid/query/rowsandcols/concrete/FrameRowsAndColumns.java Outdated Show resolved Hide resolved
processing/src/main/java/org/apache/druid/query/rowsandcols/RowsAndColumns.java Outdated
Comment on lines 162 to 163
false,
ByteBuffer.allocate(Frame.compressionBufferSize((int) frame.numBytes())),
Copy link
Member

Choose a reason for hiding this comment

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

compression is false; meanwhile a compressionBuffer is being allocated at every call - is that required?
if its needed - would it be possible to reuse the buffer later?

Copy link
Contributor Author

@sreemanamala sreemanamala Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since we are not trying to compress, I made the buffer null
Lets discuss on this, If compression can improve the performance, I can work on follow-up PR to do that work

processing/src/main/java/org/apache/druid/jackson/DruidDefaultSerializersModule.java Show resolved Hide resolved
kgyrtkirk
kgyrtkirk reviewed Sep 5, 2024
View reviewed changes
...g/src/test/java/org/apache/druid/query/rowsandcols/semantic/RowsAndColumnsDecoratorTest.java
)
);

ColumnBasedFrameRowsAndColumns frc = ColumnBasedFrameRowsAndColumnsTest.buildFrame(input);
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking if this creation could be generailzed by adding it to RowsAndColumnsTestBase#MAKERS - or that's not really usefull?

kgyrtkirk
kgyrtkirk reviewed Sep 11, 2024
View reviewed changes
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
@@ -16086,6 +16086,7 @@ public void testScanAndSortOnJoin()
.run();
}

@NotYetSupported(Modes.UNSUPPORTED_DATASOURCE)
Copy link
Member

Choose a reason for hiding this comment

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

hmm.. I wonder how did this started to happen?
its not a blocker just interested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not very sure though, but I expected this was because we used to send empty to window operator query and that has been changed now. Let me check the actual difference and comment back.

@kgyrtkirk kgyrtkirk added this to the 31.0.0 milestone Sep 16, 2024
@kgyrtkirk kgyrtkirk merged commit bb1c3c1 into apache:master Sep 17, 2024
90 checks passed
@sreemanamala sreemanamala mentioned this pull request Sep 17, 2024
Merged
10 tasks
pranavbhole pushed a commit to pranavbhole/druid that referenced this pull request Sep 17, 2024
@sreemanamala @pranavbhole
Add serde for ColumnBasedRowsAndColumns to fix window queries without…
f6998a9 
… group by (apache#16658)

Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine.
sreemanamala added a commit to sreemanamala/druid that referenced this pull request Sep 18, 2024
@sreemanamala
Add serde for ColumnBasedRowsAndColumns to fix window queries without…
70356b3 
… group by (apache#16658)

Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine.
@sreemanamala sreemanamala mentioned this pull request Sep 18, 2024
Closed
10 tasks
sreemanamala added a commit to sreemanamala/druid that referenced this pull request Sep 18, 2024
@sreemanamala
Add serde for ColumnBasedRowsAndColumns to fix window queries without…
07104e9 
… group by (apache#16658)

Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine.

(cherry picked from commit bb1c3c1)
@sreemanamala sreemanamala deleted the window-fix-nontransferable branch September 18, 2024 11:24
sreemanamala added a commit to sreemanamala/druid that referenced this pull request Sep 19, 2024
@sreemanamala
Add serde for ColumnBasedRowsAndColumns to fix window queries without…
e8a9ac3 
… group by (apache#16658)

Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine.

(cherry picked from commit bb1c3c1)
@sreemanamala sreemanamala mentioned this pull request Sep 19, 2024
Merged
kgyrtkirk pushed a commit that referenced this pull request Sep 20, 2024
@sreemanamala
Add serde for ColumnBasedRowsAndColumns to fix window queries without…
0c58f88 
… group by (#16658) (#17111)

Register a Ser-De for RowsAndColumns so that the window operator query running on leaf operators would be transferred properly on the wire. Would fix the empty response given by window queries without group by on the native engine.

(cherry picked from commit bb1c3c1)
@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

@kgyrtkirk kgyrtkirk kgyrtkirk approved these changes

Assignees
No one assigned
Labels
Area - Querying
Projects
None yet
Milestone
31.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@sreemanamala @kgyrtkirk