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

Added GENERATE_ARRAY function #12293

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Conversation

aadilkhalifa
Copy link
Contributor

@aadilkhalifa aadilkhalifa commented Jan 21, 2024

Pull request for the issue: #11786

Adding GENERATE_ARRAY function to allow generation of array of values with start, end and step value (optional).
Supported data types are int, long, float and double.
Currently does not support multi-stage query engine.

Eg. SELECT GENERATE_ARRAY(1, 5, 1) from table limit 1;

@aadilkhalifa aadilkhalifa force-pushed the array-generate branch 3 times, most recently from 5f3c103 to 736db6f Compare January 21, 2024 10:52
@aadilkhalifa aadilkhalifa changed the title [DRAFT] Added ARRAY_GENERATE function Added ARRAY_GENERATE function Jan 21, 2024
@xiangfu0
Copy link
Contributor

xiangfu0 commented Jan 21, 2024

Thanks @aadilkhalifa for raising the PR!

Can you add some integration tests under org.apache.pinot.integration.tests.custom.ArrayTest

@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2024

Codecov Report

Attention: Patch coverage is 41.93548% with 126 lines in your changes missing coverage. Please review.

Project coverage is 62.09%. Comparing base (59551e4) to head (cf17495).
Report is 738 commits behind head on master.

Files Patch % Lines
...sform/function/GenerateArrayTransformFunction.java 46.90% 84 Missing and 19 partials ⚠️
...e/pinot/common/function/scalar/ArrayFunctions.java 0.00% 20 Missing ⚠️
...r/transform/function/TransformFunctionFactory.java 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #12293      +/-   ##
============================================
+ Coverage     61.75%   62.09%   +0.34%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2559     +123     
  Lines        133233   141154    +7921     
  Branches      20636    21915    +1279     
============================================
+ Hits          82274    87646    +5372     
- Misses        44911    46873    +1962     
- Partials       6048     6635     +587     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration <0.01% <0.00%> (-0.01%) ⬇️
integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration2 0.00% <0.00%> (ø)
java-11 62.05% <41.93%> (+0.34%) ⬆️
java-21 61.96% <41.93%> (+0.33%) ⬆️
skip-bytebuffers-false 62.07% <41.93%> (+0.32%) ⬆️
skip-bytebuffers-true 61.94% <41.93%> (+34.21%) ⬆️
temurin 62.09% <41.93%> (+0.34%) ⬆️
unittests 62.08% <41.93%> (+0.34%) ⬆️
unittests1 46.69% <41.93%> (-0.20%) ⬇️
unittests2 27.62% <0.00%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -43,7 +43,7 @@ public class FunctionDefinitionRegistryTest {
// Geo functions are defined in pinot-core
"geotoh3",
// ArrayToMV and ArrayValueConstructor are placeholder functions without implementation
"arraytomv", "arrayvalueconstructor",
"arraytomv", "arrayvalueconstructor", "arraygenerate",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also update the comment ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the comment.

@@ -271,6 +271,7 @@ public enum TransformFunctionType {
OperandTypes.family(ImmutableList.of(SqlTypeFamily.ARRAY)), "vector_norm"),

ARRAY_VALUE_CONSTRUCTOR("arrayValueConstructor", "array_value_constructor"),
ARRAY_GENERATE("arrayGenerate", "array_generate"),
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the goal we want to achieve here? ARRAY_GENERATE unlike ARRAY_VALUE_CONSTRUCTOR is not a SqlStdOperatorTable entry so it will not work with multi-stage simply by adding this alone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this.

Copy link
Contributor

Choose a reason for hiding this comment

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

removing this entry will make your transform function implementation unable to register (thus unable to run / dead code).

i would suggest wait until #12302 to be checked in b/c the type system implication is complex for ARRAY_GEN
also another note to reviewer --> histogram does support a weird array gen. let's see if we can merge the 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

@aadilkhalifa
Copy link
Contributor Author

Thanks @aadilkhalifa for raising the PR!

Can you add some integration tests under org.apache.pinot.integration.tests.custom.ArrayTest

@xiangfu0 I've added the integration tests.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Shall we keep the same name as BQ: generateArray. For the scalar function version, we can do generateIntArray, generateLongArray etc.

@xiangfu0
Copy link
Contributor

Shall we keep the same name as BQ: generateArray. For the scalar function version, we can do generateIntArray, generateLongArray etc.

+1, @aadilkhalifa can you update the function names?

@aadilkhalifa aadilkhalifa force-pushed the array-generate branch 2 times, most recently from d4498fb to 488a2ee Compare March 21, 2024 08:00
@aadilkhalifa aadilkhalifa changed the title Added ARRAY_GENERATE function Added GENERATE_ARRAY function Mar 21, 2024
@aadilkhalifa
Copy link
Contributor Author

@Jackie-Jiang @xiangfu0 I've updated the names.

@xiangfu0 xiangfu0 self-assigned this Apr 9, 2024
@npawar
Copy link
Contributor

npawar commented May 23, 2024

can this be merged and issue closed?

import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.roaringbitmap.RoaringBitmap;

public class GenerateArrayTransformFunction implements TransformFunction {
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang May 23, 2024

Choose a reason for hiding this comment

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

@xiangfu0 I don't think we need a TransformFunction for this. It always only takes literal, thus always being resolved on the broker side

Copy link
Contributor

Choose a reason for hiding this comment

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

@aadilkhalifa Please take a look at this comment. Do you see this TransformFunction being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jackie-Jiang yes it is being called

@aadilkhalifa aadilkhalifa force-pushed the array-generate branch 2 times, most recently from 7d23fb7 to 6dc75af Compare June 18, 2024 09:22
@aadilkhalifa
Copy link
Contributor Author

can this be merged and issue closed?

@xiangfu0 Can this be merged?

@Jackie-Jiang Jackie-Jiang merged commit 3015b2e into apache:master Jul 15, 2024
20 checks passed
rajagopr pushed a commit to rajagopr/pinot that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants