feat: add read array support by comphead · Pull Request #1456 · apache/datafusion-comet

comphead · 2025-02-27T16:18:33Z

Which issue does this PR close?

Part of #1454 .

Rationale for this change

What changes are included in this PR?

How are these changes tested?

spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala

codecov-commenter · 2025-03-01T00:46:10Z

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 58.33%. Comparing base (f09f8af) to head (39054f3).
Report is 88 commits behind head on main.

Files with missing lines	Patch %	Lines
.../scala/org/apache/comet/serde/QueryPlanSerde.scala	71.42%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1456      +/-   ##
============================================
+ Coverage     56.12%   58.33%   +2.20%     
- Complexity      976      977       +1     
============================================
  Files           119      122       +3     
  Lines         11743    12212     +469     
  Branches       2251     2286      +35     
============================================
+ Hits           6591     7124     +533     
+ Misses         4012     3949      -63     
+ Partials       1140     1139       -1

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

andygrove · 2025-03-04T15:44:40Z

Some tests are failing due to #1289

I think the root cause is that we are trying to shuffle with arrays and Comet shuffle does not support arrays yet. We need to fall back to Spark for these shuffles.

andygrove · 2025-03-04T15:48:27Z

In CometExecRule we check to see if we support the partitioning types for the shuffle but do not check that we support the types of other columns.

@comphead Do you want to update these checks as part of this PR and see if it resolves the issue?

  case class CometExecRule(session: SparkSession) extends Rule[SparkPlan] {
    private def applyCometShuffle(plan: SparkPlan): SparkPlan = {
      plan.transformUp {
        case s: ShuffleExchangeExec
            if isCometPlan(s.child) && isCometNativeShuffleMode(conf) &&
              QueryPlanSerde.supportPartitioning(s.child.output, s.outputPartitioning)._1 =>
                 ...
              
              
        case s: ShuffleExchangeExec
            if (!s.child.supportsColumnar || isCometPlan(s.child)) && isCometJVMShuffleMode(
              conf) &&
              QueryPlanSerde.supportPartitioningTypes(s.child.output, s.outputPartitioning)._1 &&
              !isShuffleOperator(s.child) =>
                ...

comphead · 2025-03-04T18:52:46Z

In CometExecRule we check to see if we support the partitioning types for the shuffle but do not check that we support the types of other columns.

@comphead Do you want to update these checks as part of this PR and see if it resolves the issue?

  case class CometExecRule(session: SparkSession) extends Rule[SparkPlan] {
    private def applyCometShuffle(plan: SparkPlan): SparkPlan = {
      plan.transformUp {
        case s: ShuffleExchangeExec
            if isCometPlan(s.child) && isCometNativeShuffleMode(conf) &&
              QueryPlanSerde.supportPartitioning(s.child.output, s.outputPartitioning)._1 =>
                 ...
              
              
        case s: ShuffleExchangeExec
            if (!s.child.supportsColumnar || isCometPlan(s.child)) && isCometJVMShuffleMode(
              conf) &&
              QueryPlanSerde.supportPartitioningTypes(s.child.output, s.outputPartitioning)._1 &&
              !isShuffleOperator(s.child) =>
                ...

Thanks @andygrove I'll check that, you saving me hours of debugging

kazuyukitanimura · 2025-03-06T00:40:52Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

+    case s: StructType if allowComplex =>
+      s.fields.map(_.dataType).forall(supportedDataType(_, allowComplex))
+    case a: ArrayType if allowComplex =>
+      supportedDataType(a.elementType)


Do we need to pass allowComplex recursively here like supportedDataType(a.elementType, allowComplex)?

makes sense

comphead · 2025-03-07T16:19:22Z

I think the last merge brought up a new test which fails now on schema mismatch, checking this

- array_compact *** FAILED *** (204 milliseconds)
  org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 309.0 failed 1 times, most recent failure: Lost task 0.0 in stage 309.0 (TID 797) (Mac-1741305812954.local executor driver): org.apache.comet.CometNativeException: Invalid argument error: column types must match schema types, expected List(Field { name: "element", data_type: Int8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }) but found List(Field { name: "item", data_type: Int8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }) at column index 0

andygrove · 2025-03-07T23:24:58Z

I'd quite like to merge #1479 before this one so that we can be sure that no additional test failures are introduced for native_datafusion

comphead · 2025-03-18T02:50:25Z

@andygrove @kazuyukitanimura please have a second look
Nested arrays and Iceberg compat support will be added in follow up PR

kazuyukitanimura · 2025-03-18T16:25:28Z

native/core/Cargo.toml

 lazy_static = "1.4"
 assertables = "7"
 hex = "0.4.3"
+datafusion-functions-nested = "46.0.0"


can we make it to workspace = true so that we do not forget to update here when we upgrade DF

I'm not sure if it is possible to make it workspace = true, this package is optional for test only and optional packages are not allowed on workspace level.

I can include it in workspace as non optional but it will be compiled into the target binary which is probably not expected

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

andygrove · 2025-03-18T21:59:03Z

native/core/src/execution/planner.rs

+            }
+        });
+
+        runtime.block_on(async move {


It's nice to see an end-to-end test like this in the Rust project

andygrove · 2025-03-18T22:00:09Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala


-      case Literal(value, dataType) if supportedDataType(dataType, allowStruct = value == null) =>
+      case Literal(value, dataType)
+          if supportedDataType(dataType, allowComplex = value == null) =>


nit: would be nice to have a comment here explaining why allowComplex = value == null

kazuyukitanimura

Changed the description from Closes to Part of

andygrove

LGTM. Thanks @comphead

comphead · 2025-03-18T22:17:44Z

Thanks everyone

* feat: add read array support

andygrove reviewed Feb 27, 2025

View reviewed changes

spark/src/test/scala/org/apache/comet/exec/CometNativeReaderSuite.scala Outdated Show resolved Hide resolved

comphead requested a review from andygrove March 3, 2025 16:14

kazuyukitanimura reviewed Mar 6, 2025

View reviewed changes

comphead force-pushed the dev branch from ba534d4 to cc195f1 Compare March 7, 2025 00:10

comphead mentioned this pull request Mar 11, 2025

Parametrize ListArray inner field apache/datafusion#15162

Closed

comphead force-pushed the dev branch from cc195f1 to c277970 Compare March 17, 2025 23:16

comphead added 17 commits March 17, 2025 16:17

feat: add read array support

f255de6

feat: add read array support

5020860

feat: add read array support

21acd38

feat: add read array support

2a73f2d

feat: add read array support

262db2e

feat: add read array support

b86ad51

feat: add read array support

c89448c

feat: add read array support

0dc66a3

feat: add read array support

c09419d

feat: add read array support

fd6b014

feat: add read array support

26cf194

feat: add read array support

b1e070c

feat: add read array support

b66a9e9

feat: add read array support

9f79a38

feat: add read array support

7f85baa

feat: add read array support

a3a3d26

feat: add read array support

6cc3b5d

comphead force-pushed the dev branch from c277970 to 6cc3b5d Compare March 17, 2025 23:20

comphead added 3 commits March 17, 2025 16:57

feat: add read array support

68c4c7b

feat: add read array support

bbae551

feat: add read array support

7884db6

kazuyukitanimura reviewed Mar 18, 2025

View reviewed changes

feat: add read array support

39054f3

andygrove reviewed Mar 18, 2025

View reviewed changes

kazuyukitanimura approved these changes Mar 18, 2025

View reviewed changes

andygrove approved these changes Mar 18, 2025

View reviewed changes

comphead merged commit 59fae94 into apache:main Mar 18, 2025
69 checks passed

comphead mentioned this pull request Apr 2, 2025

Support MapType for native reader #1595

Closed

coderfender pushed a commit to coderfender/datafusion-comet that referenced this pull request Dec 13, 2025

feat: add read array support (apache#1456)

57e69dc

* feat: add read array support

Conversation

comphead commented Feb 27, 2025 • edited by kazuyukitanimura Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

Uh oh!

codecov-commenter commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andygrove commented Mar 4, 2025

Uh oh!

andygrove commented Mar 4, 2025

Uh oh!

comphead commented Mar 4, 2025

Uh oh!

kazuyukitanimura Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

comphead Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

comphead commented Mar 7, 2025

Uh oh!

andygrove commented Mar 7, 2025

Uh oh!

comphead commented Mar 18, 2025

Uh oh!

kazuyukitanimura Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

comphead Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andygrove Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

andygrove Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

kazuyukitanimura left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

comphead commented Mar 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

comphead commented Feb 27, 2025 •

edited by kazuyukitanimura

Loading

codecov-commenter commented Mar 1, 2025 •

edited

Loading

kazuyukitanimura left a comment •

edited

Loading