Skip to content

Conversation

@andygrove
Copy link
Member

Which issue does this PR close?

Closes #2813

Rationale for this change

What changes are included in this PR?

How are these changes tested?

@andygrove andygrove marked this pull request as ready for review November 21, 2025 23:35
@andygrove andygrove requested a review from comphead November 21, 2025 23:35
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.14%. Comparing base (f09f8af) to head (726ca21).
⚠️ Report is 736 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2815      +/-   ##
============================================
+ Coverage     56.12%   59.14%   +3.02%     
- Complexity      976     1471     +495     
============================================
  Files           119      167      +48     
  Lines         11743    15238    +3495     
  Branches       2251     2514     +263     
============================================
+ Hits           6591     9013    +2422     
- Misses         4012     4942     +930     
- Partials       1140     1283     +143     

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

}
let result_type = DataType::Decimal128(precision, scale);
ColumnarValue::Array(a) => match a.data_type() {
DataType::Int64 => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also support Int32? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this PR, I just wanted to fall back to avoid crashing. We could choose to add support in the future though

SQLConf.ANSI_ENABLED.key -> "false",
SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false",
CometConf.getExprAllowIncompatConfigKey(classOf[Sum]) -> "true",
CometConf.COMET_NATIVE_SCAN_IMPL.key -> CometConf.SCAN_NATIVE_ICEBERG_COMPAT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all settings needed for this test ? E.g. Sum and Iceberg compat look unrelated ?

checkSparkAnswerAndFallbackReason(df1, "Unsupported input data type: IntegerType")
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be useful to add a positive test too - with LongType

@comphead
Copy link
Contributor

comphead commented Dec 8, 2025

are we waiting to address any feedback on this PR?

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.

bug: Comet crashes on DataFrame API

4 participants