feat: Support Array Literal #2057
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2057 +/- ##
============================================
+ Coverage 56.12% 58.27% +2.14%
- Complexity 976 1253 +277
============================================
Files 119 141 +22
Lines 11743 13310 +1567
Branches 2251 2391 +140
============================================
+ Hits 6591 7756 +1165
- Misses 4012 4322 +310
- Partials 1140 1232 +92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -272,8 +273,7 @@ class CometArrayExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelp | |||
| } | |||
| } | |||
|
|
|||
| // https://github.com/apache/datafusion-comet/issues/1929 | |||
| ignore("array_contains - array literals") { | |||
| test("array_contains - array literals") { | |||
|
@andygrove @mbutrovich @parthchandra please take a look |
| repeated string string_values = 8; | ||
| repeated bytes bytes_values = 9; | ||
| repeated bytes decimal_values = 10; | ||
| repeated ListLiteral list_values = 11; |
There was a problem hiding this comment.
Is this for future support of lists within lists?
|
|
||
| option java_package = "org.apache.comet.serde"; | ||
|
|
||
| message ListLiteral { |
There was a problem hiding this comment.
Use an enum here. maybe?
There was a problem hiding this comment.
Thanks @parthchandra for the review, please help me to understand enum proposal? enum can define types variants
enum Type {
BOOLEAN = 0,
etc
}
and to send data across boundaries as bytes?
If it is, can it be considered as follow up PR?
There was a problem hiding this comment.
Correct. If the ListLiteral is an enum type then you will need only one field instead of multiple fields only one of which can be populated.
So
enum LiteralValueType {
BOOLEAN = 0,
...
}
message ListLiteral {
LiteralValueType value
}
Just realized that I'm not sure if it will work with nested lists though.
Maybe you can try this is in a follow up PR.
|
@parthchandra @mbutrovich please take another look |
There was a problem hiding this comment.
Thanks @comphead, just a minor CI fix I think and then this is good to go!
| // Nested literal support for native reader | ||
| // can be tracked https://github.com/apache/datafusion-comet/issues/1937 | ||
| // now supports only Array of primitive | ||
| (Seq(CometConf.SCAN_NATIVE_ICEBERG_COMPAT, CometConf.SCAN_NATIVE_DATAFUSION) |
There was a problem hiding this comment.
Why is nested literal noly supported for native reader?
Which issue does this PR close?
Closes #1977 #1929
Replaces accidentally closed #1978
Rationale for this change
What changes are included in this PR?
How are these changes tested?