You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "SemyonSinchenko (via GitHub)" <gi...@apache.org> on 2024/11/09 16:13:46 UTC
[PR] [WIP][DO-NOT-MERGE] feat: support array_insert [datafusion-comet]
SemyonSinchenko opened a new pull request, #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073
## 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
.
## What changes are included in this PR?
Work in progress
## How are these changes tested?
Work in progress
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko closed pull request #1073: feat: support array_insert
URL: https://github.com/apache/datafusion-comet/pull/1073
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#issuecomment-2489236855
Thanks for comments and suggestions!
This PR is ready for the review again.
What were changed from the last round of the review:
- I added tests for the `array_prepend` (spark 3.5+) and enabled the test for `array_append` (spark 4.0+);
- I added tests for the corner cases:
* Index is negative;
* Index is bigger than the array length;
* Index is negative and it's abs is greater than the array length;
* Test of the fallback to spark (udf as child);
* Value to insert is actually `null`;
- I fixed the native part behavior for some of the corner cases;
These tests pointed my attention to the uncovered parts of the code that I fixed and I also added couple of additional tests to the `native` part. I revisited how spark do array_insert and it is more tricky than I realized at the first glance. I added few additional comments to the native part of the implementation and fixed the behavior.
At the moment this PR is tested in multiple ways:
- Basic tests in native part that are useful because easy to debug and very fast to run;
- Basic tests in native part for the so called "legacy mode" in spark;
- Basic tests on the scala side;
- Logical tests on small data for corner cases (negative, positive, long, short, null, etc.) on the scala side;
- Tests in array_prepend and array_append that are calling `array_insert` under the hood (NULLS, different data types, etc.);
- Test of the fallback to the Spark in case when one of children is not supported by Comet;
So, it looks to me, that all the possible cases are covered and the behavior is the same like in spark.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1847400001
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -2337,4 +2337,35 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+
+ test("ArrayInsert") {
+ assume(isSpark34Plus)
+ Seq(true, false).foreach(dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "test.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled, 10000)
+ val df = spark.read
+ .parquet(path.toString)
+ .withColumn("arr", array(col("_4"), lit(null), col("_4")))
+ .withColumn("arrInsertResult", expr("array_insert(arr, 1, 1)"))
+ .withColumn("arrInsertNegativeIndexResult", expr("array_insert(arr, -1, 1)"))
+ checkSparkAnswerAndOperator(df.select("arrInsertResult"))
+ checkSparkAnswerAndOperator(df.select("arrInsertNegativeIndexResult"))
+ })
+ }
+
+ test("ArrayInsertUnsupportedArgs") {
+ // This test checks that the else branch in ArrayInsert
+ // mapping to the comet is valid and fallback to spark is working fine.
+ assume(isSpark34Plus)
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "test.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = false, 10000)
+ spark.read
+ .parquet(path.toString)
+ .withColumn("arr", array(col("_4"), lit(null), col("_4")))
+ .withColumn("idx", udf((x: Int) => x).apply(col("_4")))
+ .withColumn("arrUnsupportedArgs", expr("array_insert(arr, idx, 1)"))
Review Comment:
This code appears to construct a DataFrame but does not actually execute a query? Also there are not any assertions to check for expected behavior, so this looks like it needs some more work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1850765601
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -2337,4 +2337,35 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+
+ test("ArrayInsert") {
+ assume(isSpark34Plus)
+ Seq(true, false).foreach(dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "test.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled, 10000)
+ val df = spark.read
+ .parquet(path.toString)
+ .withColumn("arr", array(col("_4"), lit(null), col("_4")))
+ .withColumn("arrInsertResult", expr("array_insert(arr, 1, 1)"))
+ .withColumn("arrInsertNegativeIndexResult", expr("array_insert(arr, -1, 1)"))
+ checkSparkAnswerAndOperator(df.select("arrInsertResult"))
+ checkSparkAnswerAndOperator(df.select("arrInsertNegativeIndexResult"))
+ })
+ }
+
+ test("ArrayInsertUnsupportedArgs") {
+ // This test checks that the else branch in ArrayInsert
+ // mapping to the comet is valid and fallback to spark is working fine.
+ assume(isSpark34Plus)
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "test.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = false, 10000)
+ spark.read
+ .parquet(path.toString)
+ .withColumn("arr", array(col("_4"), lit(null), col("_4")))
+ .withColumn("idx", udf((x: Int) => x).apply(col("_4")))
+ .withColumn("arrUnsupportedArgs", expr("array_insert(arr, idx, 1)"))
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#issuecomment-2475558683
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/1073?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `59.09091%` with `9 lines` in your changes missing coverage. Please review.
> Project coverage is 34.18%. Comparing base [(`845b654`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/845b654c12f80c02890371325ed3fb360a29636b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`8b58d8d`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/8b58d8d7ba42e3dbd1952ee32b59bdca8540fe1b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 18 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion-comet/pull/1073?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../scala/org/apache/comet/serde/QueryPlanSerde.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/1073?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2Fserde%2FQueryPlanSerde.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9zZXJkZS9RdWVyeVBsYW5TZXJkZS5zY2FsYQ==) | 59.09% | [7 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/1073?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1073 +/- ##
============================================
- Coverage 34.46% 34.18% -0.29%
+ Complexity 888 883 -5
============================================
Files 113 113
Lines 43580 43355 -225
Branches 9658 9488 -170
============================================
- Hits 15021 14821 -200
- Misses 25507 25627 +120
+ Partials 3052 2907 -145
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/1073?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "NoeB (via GitHub)" <gi...@apache.org>.
NoeB commented on PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#issuecomment-2484898030
I am not sure if this should be done together with this PR but it would some add "free" tests. Spark introduced with 3.5 array_prepend which it implements with array_insert and starting 4.0 it also implements array_append with array_insert. If you want you can copy the array_append tests and replace array_append with array_prepend for Spark 3.5+ and enable the array_append tests for spark 4.0. You can also ignore this comment if you do not agree or if it leads to unrelated errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1862550710
##########
native/spark-expr/src/list.rs:
##########
@@ -413,14 +426,297 @@ impl PartialEq<dyn Any> for GetArrayStructFields {
}
}
+#[derive(Debug, Hash)]
+pub struct ArrayInsert {
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+}
+
+impl ArrayInsert {
+ pub fn new(
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+ ) -> Self {
+ Self {
+ src_array_expr,
+ pos_expr,
+ item_expr,
+ legacy_negative_index,
+ }
+ }
+}
+
+impl PhysicalExpr for ArrayInsert {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> DataFusionResult<DataType> {
+ match self.src_array_expr.data_type(input_schema)? {
+ DataType::List(field) => Ok(DataType::List(field)),
+ DataType::LargeList(field) => Ok(DataType::LargeList(field)),
+ data_type => Err(DataFusionError::Internal(format!(
+ "Unexpected data type in ArrayInsert: {:?}",
+ data_type
+ ))),
+ }
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> DataFusionResult<bool> {
+ self.src_array_expr.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> DataFusionResult<ColumnarValue> {
+ let pos_value = self
+ .pos_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+
+ // Spark supports only IntegerType (Int32):
+ // https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4737
+ if !matches!(pos_value.data_type(), DataType::Int32) {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected index data type in ArrayInsert: {:?}, expected type is Int32",
+ pos_value.data_type()
+ )));
+ }
+
+ // Check that src array is actually an array and get it's value type
+ let src_value = self
+ .src_array_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+ let src_element_type = match src_value.data_type() {
+ DataType::List(field) => field.data_type(),
+ DataType::LargeList(field) => field.data_type(),
+ data_type => {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected src array type in ArrayInsert: {:?}",
+ data_type
+ )))
+ }
Review Comment:
The difference I think is that a `LargeList` can store more than `Integer.MAX_VALUE` entries in _all rows in a single batch_, so if you have multiple Spark rows all with the max num of rows supported, it wouldn't fit into an Arrow `List` array. That would probably need to be supported elsewhere, but it may be worth keeping the `LargeList` handling around in case that scenario is supported? And other DataFusion expressions might return a `LargeList` even if it doesn't come directly from Spark? Does the native Parquet reader ever use a `LargeList`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1851687830
##########
native/spark-expr/src/list.rs:
##########
@@ -413,14 +426,297 @@ impl PartialEq<dyn Any> for GetArrayStructFields {
}
}
+#[derive(Debug, Hash)]
+pub struct ArrayInsert {
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+}
+
+impl ArrayInsert {
+ pub fn new(
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+ ) -> Self {
+ Self {
+ src_array_expr,
+ pos_expr,
+ item_expr,
+ legacy_negative_index,
+ }
+ }
+}
+
+impl PhysicalExpr for ArrayInsert {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> DataFusionResult<DataType> {
+ match self.src_array_expr.data_type(input_schema)? {
+ DataType::List(field) => Ok(DataType::List(field)),
+ DataType::LargeList(field) => Ok(DataType::LargeList(field)),
+ data_type => Err(DataFusionError::Internal(format!(
+ "Unexpected data type in ArrayInsert: {:?}",
+ data_type
+ ))),
+ }
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> DataFusionResult<bool> {
+ self.src_array_expr.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> DataFusionResult<ColumnarValue> {
+ let pos_value = self
+ .pos_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+
+ // Spark supports only IntegerType (Int32):
+ // https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4737
+ if !matches!(pos_value.data_type(), DataType::Int32) {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected index data type in ArrayInsert: {:?}, expected type is Int32",
+ pos_value.data_type()
+ )));
+ }
+
+ // Check that src array is actually an array and get it's value type
+ let src_value = self
+ .src_array_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+ let src_element_type = match src_value.data_type() {
+ DataType::List(field) => field.data_type(),
+ DataType::LargeList(field) => field.data_type(),
+ data_type => {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected src array type in ArrayInsert: {:?}",
+ data_type
+ )))
+ }
Review Comment:
@andygrove Thanks for the suggestion!
I moved a checking of the array type (and the exception logic) to the method:
```rs
pub fn array_type(&self, data_type: &DataType) -> DataFusionResult<DataType> {
match data_type {
DataType::List(field) => Ok(DataType::List(Arc::clone(field))),
DataType::LargeList(field) => Ok(DataType::LargeList(Arc::clone(field))),
data_type => {
return Err(DataFusionError::Internal(format!(
"Unexpected src array type in ArrayInsert: {:?}",
data_type
)))
}
}
}
```
It allows at least to avoid returning the same error multiple time. Is it what you suggested? Or should I move this method to a helper function and refactor also `GerArrayStructField` to use such a function?
P.S. Sorry for the stupid question... But can you please explain to me why we always check both `List` and `LargeList`, while Apache Spark only supports `i32` indexes for arrays (max length is `Integer.MAX_VALUE - 15`), which is the case of `List` to my understanding? All the code in the `list.rs` might become a bit simpler if we make it non-generic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#issuecomment-2474910059
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1851687830
##########
native/spark-expr/src/list.rs:
##########
@@ -413,14 +426,297 @@ impl PartialEq<dyn Any> for GetArrayStructFields {
}
}
+#[derive(Debug, Hash)]
+pub struct ArrayInsert {
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+}
+
+impl ArrayInsert {
+ pub fn new(
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+ ) -> Self {
+ Self {
+ src_array_expr,
+ pos_expr,
+ item_expr,
+ legacy_negative_index,
+ }
+ }
+}
+
+impl PhysicalExpr for ArrayInsert {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> DataFusionResult<DataType> {
+ match self.src_array_expr.data_type(input_schema)? {
+ DataType::List(field) => Ok(DataType::List(field)),
+ DataType::LargeList(field) => Ok(DataType::LargeList(field)),
+ data_type => Err(DataFusionError::Internal(format!(
+ "Unexpected data type in ArrayInsert: {:?}",
+ data_type
+ ))),
+ }
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> DataFusionResult<bool> {
+ self.src_array_expr.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> DataFusionResult<ColumnarValue> {
+ let pos_value = self
+ .pos_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+
+ // Spark supports only IntegerType (Int32):
+ // https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4737
+ if !matches!(pos_value.data_type(), DataType::Int32) {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected index data type in ArrayInsert: {:?}, expected type is Int32",
+ pos_value.data_type()
+ )));
+ }
+
+ // Check that src array is actually an array and get it's value type
+ let src_value = self
+ .src_array_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+ let src_element_type = match src_value.data_type() {
+ DataType::List(field) => field.data_type(),
+ DataType::LargeList(field) => field.data_type(),
+ data_type => {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected src array type in ArrayInsert: {:?}",
+ data_type
+ )))
+ }
Review Comment:
@andygrove Thanks for the suggestion!
I moved a checking of the array type (and the exception logic) to the method:
```rs
pub fn array_type(&self, data_type: &DataType) -> DataFusionResult<DataType> {
match data_type {
DataType::List(field) => Ok(DataType::List(Arc::clone(field))),
DataType::LargeList(field) => Ok(DataType::LargeList(Arc::clone(field))),
data_type => {
return Err(DataFusionError::Internal(format!(
"Unexpected src array type in ArrayInsert: {:?}",
data_type
)))
}
}
}
```
It allows at least to avoid returning the same error multiple time. Is it what you suggested? Or should I move this method to a helper function and refactor also `GerArrayStructField` to use such a function?
P.S. Sorry for the stupid question... But can you please explain to me why we always check both `List` and `LargeList`, while Apache Spark only supports `i32` indexes for arrays (max length is `Integer.MAX_VALUE - 15`), which is the case of `List` to my understanding? All the code in the `list.rs` might become a bit simpler if we make it non-generic (it also makes implementation of other missing methods like `array_zip` simpler).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#issuecomment-2480564724
This PR is ready for the review. The only failed check failed due to the internal GHA error:
```sh
GitHub Actions has encountered an internal error when running your job.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1853910995
##########
native/spark-expr/src/list.rs:
##########
@@ -413,14 +426,297 @@ impl PartialEq<dyn Any> for GetArrayStructFields {
}
}
+#[derive(Debug, Hash)]
+pub struct ArrayInsert {
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+}
+
+impl ArrayInsert {
+ pub fn new(
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+ ) -> Self {
+ Self {
+ src_array_expr,
+ pos_expr,
+ item_expr,
+ legacy_negative_index,
+ }
+ }
+}
+
+impl PhysicalExpr for ArrayInsert {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> DataFusionResult<DataType> {
+ match self.src_array_expr.data_type(input_schema)? {
+ DataType::List(field) => Ok(DataType::List(field)),
+ DataType::LargeList(field) => Ok(DataType::LargeList(field)),
+ data_type => Err(DataFusionError::Internal(format!(
+ "Unexpected data type in ArrayInsert: {:?}",
+ data_type
+ ))),
+ }
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> DataFusionResult<bool> {
+ self.src_array_expr.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> DataFusionResult<ColumnarValue> {
+ let pos_value = self
+ .pos_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+
+ // Spark supports only IntegerType (Int32):
+ // https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4737
+ if !matches!(pos_value.data_type(), DataType::Int32) {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected index data type in ArrayInsert: {:?}, expected type is Int32",
+ pos_value.data_type()
+ )));
+ }
+
+ // Check that src array is actually an array and get it's value type
+ let src_value = self
+ .src_array_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+ let src_element_type = match src_value.data_type() {
+ DataType::List(field) => field.data_type(),
+ DataType::LargeList(field) => field.data_type(),
+ data_type => {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected src array type in ArrayInsert: {:?}",
+ data_type
+ )))
+ }
Review Comment:
That's a good question. I wonder if the existing code for LargeList is actually being tested. It would be interesting to try removing it and see if there are any regressions. It makes sense to only handle List if Spark only supports i32 indexes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1847393237
##########
native/spark-expr/src/list.rs:
##########
@@ -413,14 +426,286 @@ impl PartialEq<dyn Any> for GetArrayStructFields {
}
}
+#[derive(Debug, Hash)]
+pub struct ArrayInsert {
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+}
+
+impl ArrayInsert {
+ pub fn new(
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+ ) -> Self {
+ Self {
+ src_array_expr,
+ pos_expr,
+ item_expr,
+ legacy_negative_index,
+ }
+ }
+}
+
+impl PhysicalExpr for ArrayInsert {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> DataFusionResult<DataType> {
+ match self.src_array_expr.data_type(input_schema)? {
+ DataType::List(field) => Ok(DataType::List(field)),
+ DataType::LargeList(field) => Ok(DataType::LargeList(field)),
+ data_type => Err(DataFusionError::Internal(format!(
+ "Unexpected data type in ArrayInsert: {:?}",
+ data_type
+ ))),
+ }
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> DataFusionResult<bool> {
+ self.src_array_expr.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> DataFusionResult<ColumnarValue> {
+ let pos_value = self
+ .pos_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+
+ // Spark supports only IntegerType (Int32):
+ // https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4737
+ match pos_value.data_type() {
+ DataType::Int32 => {}
+ data_type => {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected index data type in ArrayInsert: {:?}, expected type is Int32",
+ data_type
+ )))
+ }
+ }
Review Comment:
nit: this can be more concise using the `matches!` macro.
```suggestion
if !matches!(pos_value.data_type(), DataType::Int32) {
return Err(DataFusionError::Internal(format!(
"Unexpected index data type in ArrayInsert: {:?}, expected type is Int32",
pos_value.data_type()
)))
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1851080830
##########
native/spark-expr/src/list.rs:
##########
@@ -413,14 +426,297 @@ impl PartialEq<dyn Any> for GetArrayStructFields {
}
}
+#[derive(Debug, Hash)]
+pub struct ArrayInsert {
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+}
+
+impl ArrayInsert {
+ pub fn new(
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+ ) -> Self {
+ Self {
+ src_array_expr,
+ pos_expr,
+ item_expr,
+ legacy_negative_index,
+ }
+ }
+}
+
+impl PhysicalExpr for ArrayInsert {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> DataFusionResult<DataType> {
+ match self.src_array_expr.data_type(input_schema)? {
+ DataType::List(field) => Ok(DataType::List(field)),
+ DataType::LargeList(field) => Ok(DataType::LargeList(field)),
+ data_type => Err(DataFusionError::Internal(format!(
+ "Unexpected data type in ArrayInsert: {:?}",
+ data_type
+ ))),
+ }
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> DataFusionResult<bool> {
+ self.src_array_expr.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> DataFusionResult<ColumnarValue> {
+ let pos_value = self
+ .pos_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+
+ // Spark supports only IntegerType (Int32):
+ // https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4737
+ if !matches!(pos_value.data_type(), DataType::Int32) {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected index data type in ArrayInsert: {:?}, expected type is Int32",
+ pos_value.data_type()
+ )));
+ }
+
+ // Check that src array is actually an array and get it's value type
+ let src_value = self
+ .src_array_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+ let src_element_type = match src_value.data_type() {
+ DataType::List(field) => field.data_type(),
+ DataType::LargeList(field) => field.data_type(),
+ data_type => {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected src array type in ArrayInsert: {:?}",
+ data_type
+ )))
+ }
Review Comment:
minor nit: this logic for extracting a list type is repeated a few times and could be factored out into a function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] [WIP][DO-NOT-MERGE] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#issuecomment-2466311537
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] [WIP][DO-NOT-MERGE] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#issuecomment-2468325238
@andygrove Sorry for tagging but I have questions about the ticket (`array_insert`).
1. `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](https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4736-L4760) `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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#issuecomment-2489237408
> I am not sure if this should be done together with this PR but it would some add "free" tests. Spark introduced with 3.5 array_prepend which it implements with array_insert and starting 4.0 it also implements array_append with array_insert. If you want you can copy the array_append tests and replace array_append with array_prepend for Spark 3.5+ and enable the array_append tests for spark 4.0. You can also ignore this comment if you do not agree or if it leads to unrelated errors.
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1862550710
##########
native/spark-expr/src/list.rs:
##########
@@ -413,14 +426,297 @@ impl PartialEq<dyn Any> for GetArrayStructFields {
}
}
+#[derive(Debug, Hash)]
+pub struct ArrayInsert {
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+}
+
+impl ArrayInsert {
+ pub fn new(
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+ ) -> Self {
+ Self {
+ src_array_expr,
+ pos_expr,
+ item_expr,
+ legacy_negative_index,
+ }
+ }
+}
+
+impl PhysicalExpr for ArrayInsert {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> DataFusionResult<DataType> {
+ match self.src_array_expr.data_type(input_schema)? {
+ DataType::List(field) => Ok(DataType::List(field)),
+ DataType::LargeList(field) => Ok(DataType::LargeList(field)),
+ data_type => Err(DataFusionError::Internal(format!(
+ "Unexpected data type in ArrayInsert: {:?}",
+ data_type
+ ))),
+ }
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> DataFusionResult<bool> {
+ self.src_array_expr.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> DataFusionResult<ColumnarValue> {
+ let pos_value = self
+ .pos_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+
+ // Spark supports only IntegerType (Int32):
+ // https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4737
+ if !matches!(pos_value.data_type(), DataType::Int32) {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected index data type in ArrayInsert: {:?}, expected type is Int32",
+ pos_value.data_type()
+ )));
+ }
+
+ // Check that src array is actually an array and get it's value type
+ let src_value = self
+ .src_array_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+ let src_element_type = match src_value.data_type() {
+ DataType::List(field) => field.data_type(),
+ DataType::LargeList(field) => field.data_type(),
+ data_type => {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected src array type in ArrayInsert: {:?}",
+ data_type
+ )))
+ }
Review Comment:
The difference I think is that a `LargeList` can store more than `Integer.MAX_VALUE` entries in _all rows in a single batch_, so if you have multiple Spark rows all with the max num of rows supported, it wouldn't fit into an Arrow `List` array. That would probably need to be supported elsewhere, but it may be worth keeping the `LargeList` handling around in case that scenario is supported. And other DataFusion expressions might return a `LargeList` even if it doesn't come directly from Spark? Does the native Parquet reader ever use a `LargeList`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1862564708
##########
native/spark-expr/src/list.rs:
##########
@@ -413,14 +426,297 @@ impl PartialEq<dyn Any> for GetArrayStructFields {
}
}
+#[derive(Debug, Hash)]
+pub struct ArrayInsert {
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+}
+
+impl ArrayInsert {
+ pub fn new(
+ src_array_expr: Arc<dyn PhysicalExpr>,
+ pos_expr: Arc<dyn PhysicalExpr>,
+ item_expr: Arc<dyn PhysicalExpr>,
+ legacy_negative_index: bool,
+ ) -> Self {
+ Self {
+ src_array_expr,
+ pos_expr,
+ item_expr,
+ legacy_negative_index,
+ }
+ }
+}
+
+impl PhysicalExpr for ArrayInsert {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, input_schema: &Schema) -> DataFusionResult<DataType> {
+ match self.src_array_expr.data_type(input_schema)? {
+ DataType::List(field) => Ok(DataType::List(field)),
+ DataType::LargeList(field) => Ok(DataType::LargeList(field)),
+ data_type => Err(DataFusionError::Internal(format!(
+ "Unexpected data type in ArrayInsert: {:?}",
+ data_type
+ ))),
+ }
+ }
+
+ fn nullable(&self, input_schema: &Schema) -> DataFusionResult<bool> {
+ self.src_array_expr.nullable(input_schema)
+ }
+
+ fn evaluate(&self, batch: &RecordBatch) -> DataFusionResult<ColumnarValue> {
+ let pos_value = self
+ .pos_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+
+ // Spark supports only IntegerType (Int32):
+ // https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L4737
+ if !matches!(pos_value.data_type(), DataType::Int32) {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected index data type in ArrayInsert: {:?}, expected type is Int32",
+ pos_value.data_type()
+ )));
+ }
+
+ // Check that src array is actually an array and get it's value type
+ let src_value = self
+ .src_array_expr
+ .evaluate(batch)?
+ .into_array(batch.num_rows())?;
+ let src_element_type = match src_value.data_type() {
+ DataType::List(field) => field.data_type(),
+ DataType::LargeList(field) => field.data_type(),
+ data_type => {
+ return Err(DataFusionError::Internal(format!(
+ "Unexpected src array type in ArrayInsert: {:?}",
+ data_type
+ )))
+ }
Review Comment:
Thanks for the explanation!
You are right, I will close #1118 then
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support array_insert [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1073:
URL: https://github.com/apache/datafusion-comet/pull/1073#discussion_r1841235680
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -2313,4 +2313,21 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+
+ test("ArrayInsert") {
Review Comment:
You can add an `assume` call here to skip the test for older Spark versions.
```suggestion
test("ArrayInsert") {
assume(isSpark34Plus)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org