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

feat: support array_insert #1073

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

SemyonSinchenko
Copy link
Member

@SemyonSinchenko SemyonSinchenko commented Nov 9, 2024

Which issue does this PR close?

Related to #1042

array_insert: SELECT array_insert(array(1, 2, 3, 4), 5, 5)

Rationale for this change

As described in #1042

What changes are included in this PR?

  • QueryPlanSerde.scala: I added an additional case for the array insert;
  • expr.proto: I added a new message for the ArrayInsert;
  • planner.rs: I added a case for the array_insert;
  • list.rs:
    • I added a new ArrayInsert struct;
    • I implemented PhysicalExpr, Display and PartialExpr for it;
    • The main logic of insertion is in fn array_insert

How are these changes tested?

At the moment I added a simple tests for fn array_insert and a test for QueryPlanSerde.

@SemyonSinchenko
Copy link
Member Author

As I was able to realize, array_insert does not supported in datafusion. Is the list.rs a good place to have an implementation of ArrayInsert and PhysicalExpr for it?

@SemyonSinchenko
Copy link
Member Author

SemyonSinchenko commented Nov 11, 2024

@andygrove Sorry for tagging but I have questions about the ticket (array_insert).

  1. [RESOLVED] array_insert was added in spark 3.4, so all the 3.3.x tests are obviously failed. I checked and it looks like the EoL for 3.3 is about the end of 2024. Technically I think I can try to workaround tests in 3.3.x by reflection API, my question is mostly should I do it due to soon EoL of the 3.3.x?
  2. array_insert is not supported in DataFusion. I made an implementation (and it looks like it works, except negative indices and corner cases). Is the list.rs a good place for it? Or should I move my code somewhere else?
  3. Spark does not support anything except Int32 for position argument, is it OK if I will support only int32 too? In theory, other types can be supported too, but I'm still trying to realize how to achieve it and it may become complex...

Thanks in advance! That is my first serious attempt to contribute to the project, so sorry If I'm annoying.

@SemyonSinchenko SemyonSinchenko changed the title [WIP][DO-NOT-MERGE] feat: support array_insert [WIP] feat: support array_insert Nov 13, 2024
@SemyonSinchenko SemyonSinchenko changed the title [WIP] feat: support array_insert feat: support array_insert Nov 13, 2024
@SemyonSinchenko SemyonSinchenko changed the title feat: support array_insert [WIP] feat: support array_insert Nov 13, 2024
- added test for the negative index
- added test for the legacy spark mode
@SemyonSinchenko SemyonSinchenko changed the title [WIP] feat: support array_insert feat: support array_insert Nov 13, 2024
@SemyonSinchenko SemyonSinchenko marked this pull request as ready for review November 13, 2024 18:04
@andygrove
Copy link
Member

Thanks, @SemyonSinchenko. I think it's fine to skip the test for Spark 3.3. I plan on reviewing this PR in more detail tomorrow, but it looks good from an initial read.

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 59.09091% with 9 lines in your changes missing coverage. Please review.

Project coverage is 34.27%. Comparing base (845b654) to head (8b58d8d).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
.../scala/org/apache/comet/serde/QueryPlanSerde.scala 59.09% 7 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1073      +/-   ##
============================================
- Coverage     34.46%   34.27%   -0.20%     
  Complexity      888      888              
============================================
  Files           113      113              
  Lines         43580    43355     -225     
  Branches       9658     9488     -170     
============================================
- Hits          15021    14860     -161     
- Misses        25507    25596      +89     
+ Partials       3052     2899     -153     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants