feat: enable iceberg compat tests, more tests for complex types #1550

comphead · 2025-03-19T01:37:41Z

Which issue does this PR close?

Part of #1454 .

Rationale for this change

Enable Iceberg compat type tests, add more tests for complex types

What changes are included in this PR?

How are these changes tested?

comphead · 2025-03-19T01:40:38Z

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

@@ -63,4 +61,67 @@ class CometNativeReaderSuite extends CometTestBase with AdaptiveSparkPlanHelper
        |""".stripMargin,
      "select arr from tbl")
  }
+
+  /*


Followup ticket to fix schema issues for native reader - read STRUCT of ARRAY fields #1551

codecov-commenter · 2025-03-19T02:34:53Z

Codecov Report

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

Project coverage is 58.53%. Comparing base (f09f8af) to head (0e9423a).
Report is 104 commits behind head on main.

Files with missing lines	Patch %	Lines
.../scala/org/apache/comet/serde/QueryPlanSerde.scala	28.57%	4 Missing and 1 partial ⚠️
...org/apache/comet/CometSparkSessionExtensions.scala	66.66%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1550      +/-   ##
============================================
+ Coverage     56.12%   58.53%   +2.40%     
- Complexity      976      977       +1     
============================================
  Files           119      122       +3     
  Lines         11743    12237     +494     
  Branches       2251     2279      +28     
============================================
+ Hits           6591     7163     +572     
+ Misses         4012     3944      -68     
+ Partials       1140     1130      -10

☔ 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.

comphead · 2025-03-19T16:35:30Z

Not sure why test failed, it is okay on local

parthchandra

Great tests @comphead
Do you think we need to add some cases with one more level of nesting -

  array
    +- struct 
            +- array

and

 struct
   +- array
           +- struct

or would that be overkill?

comphead · 2025-03-20T23:13:42Z

Great tests @comphead Do you think we need to add some cases with one more level of nesting -
  array
    +- struct 
            +- array
and
 struct
   +- array
           +- struct
or would that be overkill?

Thanks @parthchandra I'll try to add them once I fx other tests currently failing

UPD: it can be done once #1551 is fixed

parthchandra · 2025-03-21T23:05:12Z

 struct
   +- array
           +- struct

fyi, currently some tests in Spark SQL (e.g. "add a nested column at the end of the leaf struct column") fail because of structs like this with.

Caused by: org.apache.comet.CometNativeException: no entry found for key
[info]         at comet::errors::init::{{closure}}(__internal__:0)
[info]         at std::panicking::rust_panic_with_hook(__internal__:0)
[info]         at std::panicking::begin_panic_handler::{{closure}}(__internal__:0)
[info]         at std::sys::backtrace::__rust_end_short_backtrace(__internal__:0)
[info]         at rust_begin_unwind(__internal__:0)
[info]         at core::panicking::panic_fmt(__internal__:0)
[info]         at core::option::expect_failed(__internal__:0)
[info]         at comet::parquet::parquet_support::cast_struct_to_struct(__internal__:0)
[info]         at comet::parquet::parquet_support::cast_array(__internal__:0)
[info]         at comet::parquet::parquet_support::cast_struct_to_struct(__internal__:0)
[info]         at comet::parquet::parquet_support::cast_array(__internal__:0)
[info]         at comet::parquet::parquet_support::spark_parquet_convert(__internal__:0)
[info]         at <core::iter::adapters::GenericShunt<I,R> as core::iter::traits::iterator::Iterator>::next(__internal__:0)
[info]         at <comet::parquet::schema_adapter::SchemaMapping as datafusion::datasource::schema_adapter::SchemaMapper>::map_batch(__internal__:0)
[info]         at <futures_util::stream::stream::map::Map<St,F> as futures_core::stream::Stream>::poll_next(__internal__:0)
[info]         at <datafusion_datasource::file_stream::FileStream as futures_core::stream::Stream>::poll_next(__internal__:0)
[info]         at Java_org_apache_comet_parquet_Native_readNextRecordBatch(__internal__:0)
[info]         at <unknown>(__internal__:0)
[info] 	at org.apache.comet.parquet.Native.readNextRecordBatch(Native Method)

comphead · 2025-03-22T00:02:03Z

 struct
   +- array
           +- struct

fyi, currently some tests in Spark SQL (e.g. "add a nested column at the end of the leaf struct column") fail because of structs like this with.

Caused by: org.apache.comet.CometNativeException: no entry found for key
[info]         at comet::errors::init::{{closure}}(__internal__:0)
[info]         at std::panicking::rust_panic_with_hook(__internal__:0)
[info]         at std::panicking::begin_panic_handler::{{closure}}(__internal__:0)
[info]         at std::sys::backtrace::__rust_end_short_backtrace(__internal__:0)
[info]         at rust_begin_unwind(__internal__:0)
[info]         at core::panicking::panic_fmt(__internal__:0)
[info]         at core::option::expect_failed(__internal__:0)
[info]         at comet::parquet::parquet_support::cast_struct_to_struct(__internal__:0)
[info]         at comet::parquet::parquet_support::cast_array(__internal__:0)
[info]         at comet::parquet::parquet_support::cast_struct_to_struct(__internal__:0)
[info]         at comet::parquet::parquet_support::cast_array(__internal__:0)
[info]         at comet::parquet::parquet_support::spark_parquet_convert(__internal__:0)
[info]         at <core::iter::adapters::GenericShunt<I,R> as core::iter::traits::iterator::Iterator>::next(__internal__:0)
[info]         at <comet::parquet::schema_adapter::SchemaMapping as datafusion::datasource::schema_adapter::SchemaMapper>::map_batch(__internal__:0)
[info]         at <futures_util::stream::stream::map::Map<St,F> as futures_core::stream::Stream>::poll_next(__internal__:0)
[info]         at <datafusion_datasource::file_stream::FileStream as futures_core::stream::Stream>::poll_next(__internal__:0)
[info]         at Java_org_apache_comet_parquet_Native_readNextRecordBatch(__internal__:0)
[info]         at <unknown>(__internal__:0)
[info] 	at org.apache.comet.parquet.Native.readNextRecordBatch(Native Method)

appreciate if you can mention those tests in #1551 so we can double check if they can pass

comphead · 2025-03-25T15:54:13Z

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

+            supportedDataType(
+              a.dataType,
+              allowComplex =
+                usingDataFusionParquetExec(conf) || op.isInstanceOf[CometSparkToColumnarExec])) =>


@mbutrovich I think this check require attention.
It needs to work for CometSparkToColumnarExec as well

kazuyukitanimura

Just one commment

kazuyukitanimura · 2025-03-25T21:59:24Z

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

+            supportedDataType(
+              a.dataType,
+              allowComplex =
+                usingDataFusionParquetExec(conf) || CometConf.COMET_CONVERT_FROM_PARQUET_ENABLED


Why COMET_CONVERT_FROM_* is allowed only here but not other places?

comphead requested a review from kazuyukitanimura March 19, 2025 01:38

comphead commented Mar 19, 2025

View reviewed changes

comphead mentioned this pull request Mar 19, 2025

feat: fix schema issues for native reader - read STRUCT of ARRAY fields #1551

Open

comphead requested a review from parthchandra March 19, 2025 17:50

parthchandra reviewed Mar 20, 2025

View reviewed changes

comphead added 11 commits March 25, 2025 08:18

feat: enable iceberg compat tests, more tests for complex types

3edf9c6

feat: enable iceberg compat tests, more tests for complex types

7787de8

feat: enable iceberg compat tests, more tests for complex types

08e9aa3

feat: enable iceberg compat tests, more tests for complex types

dde54d9

feat: enable iceberg compat tests, more tests for complex types

363989f

feat: enable iceberg compat tests, more tests for complex types

9ca5acf

feat: enable iceberg compat tests, more tests for complex types

9802c10

feat: enable iceberg compat tests, more tests for complex types

5129198

feat: enable iceberg compat tests, more tests for complex types

e0c6ff0

feat: enable iceberg compat tests, more tests for complex types

4747f7b

feat: enable iceberg compat tests, more tests for complex types

ba0d446

comphead commented Mar 25, 2025

View reviewed changes

feat: enable iceberg compat tests, more tests for complex types

Loading
Loading status checks…

0452d0f

comphead force-pushed the dev branch from a8ad249 to 0452d0f Compare March 25, 2025 16:03

feat: enable iceberg compat tests, more tests for complex types

Loading
Loading status checks…

c816999

kazuyukitanimura approved these changes Mar 25, 2025

View reviewed changes

comphead added 3 commits March 25, 2025 16:56

feat: enable iceberg compat tests, more tests for complex types

Loading
Loading status checks…

e90bc9e

feat: enable iceberg compat tests, more tests for complex types

Loading
Loading status checks…

9ef8075

feat: enable iceberg compat tests, more tests for complex types

Loading
Loading status checks…

0e9423a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable iceberg compat tests, more tests for complex types #1550

feat: enable iceberg compat tests, more tests for complex types #1550

comphead commented Mar 19, 2025

comphead Mar 19, 2025 •

edited

Loading

codecov-commenter commented Mar 19, 2025 •

edited

Loading

comphead commented Mar 19, 2025

parthchandra left a comment

comphead commented Mar 20, 2025 •

edited

Loading

parthchandra commented Mar 21, 2025

comphead commented Mar 22, 2025

comphead Mar 25, 2025

kazuyukitanimura left a comment

kazuyukitanimura Mar 25, 2025

feat: enable iceberg compat tests, more tests for complex types #1550

Are you sure you want to change the base?

feat: enable iceberg compat tests, more tests for complex types #1550

Conversation

comphead commented Mar 19, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

comphead Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Mar 19, 2025 • edited Loading

Codecov Report

comphead commented Mar 19, 2025

parthchandra left a comment

Choose a reason for hiding this comment

comphead commented Mar 20, 2025 • edited Loading

parthchandra commented Mar 21, 2025

comphead commented Mar 22, 2025

comphead Mar 25, 2025

Choose a reason for hiding this comment

kazuyukitanimura left a comment

Choose a reason for hiding this comment

kazuyukitanimura Mar 25, 2025

Choose a reason for hiding this comment

comphead Mar 19, 2025 •

edited

Loading

codecov-commenter commented Mar 19, 2025 •

edited

Loading

comphead commented Mar 20, 2025 •

edited

Loading