You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mbutrovich (via GitHub)" <gi...@apache.org> on 2025/02/11 15:57:40 UTC
[PR] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
mbutrovich opened a new pull request, #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Reduces number of test failures in `native_datafusion` and `native_iceberg_compat` modes:
- native_datafusion: succeeded 651, failed 27, canceled 2, ignored 54, pending 0
- native_iceberg_compat: succeeded 640, failed 38, canceled 2, ignored 54, pending 0
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
After staring at conversions and hoisting code from arrow-rs to debug, it turns out we can just use arrow's cast in this case.
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Ran `make test-jvm` for `native_datafusion` and `native_iceberg_compat` modes.
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1951907716
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
+ _ => array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?,
Review Comment:
Can you log an issue to review the use of `array_with_timezone` ? The method was written to cast timestamps provided by Spark while here we are casting timestamps provided by Parquet and there might be subtle cases where the behavior is different. As long as the unit tests pass, I believe we are safe for all use cases, but it would be a good idea to review.
--
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] fix: Reduce cast.rs and utils.rs logic from parquet_support.rs for experimental native scans [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2660195143
@parthchandra any other comments? otherwise I can merge this
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1951123998
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
Review Comment:
This was causing one of the duplicate time zone changes.
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1952626361
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
Review Comment:
In fact maybe I'll remove that in this PR if nothing breaks.
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2656383868
More info -
with `native_iceberg_compat`:
```
from_type: Timestamp(Nanosecond, None) to_type: Timestamp(Microsecond, Some("America/Los_Angeles"))
array: PrimitiveArray<Timestamp(Nanosecond, None)>
[
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
],
casted: Ok(PrimitiveArray<Timestamp(Microsecond, Some("America/Los_Angeles"))>
[
2020-01-01T09:02:03.123456-08:00,
2020-01-01T09:02:03.123456-08:00,
2020-01-01T09:02:03.123456-08:00,
2020-01-01T09:02:03.123456-08:00,
2020-01-01T09:02:03.123456-08:00,
2020-01-01T09:02:03.123456-08:00,
2020-01-01T09:02:03.123456-08:00,
2020-01-01T09:02:03.123456-08:00,
2020-01-01T09:02:03.123456-08:00,
2020-01-01T09:02:03.123456-08:00,
])
```
with `native_datafusion`:
from_type: Timestamp(Nanosecond, None) to_type: Timestamp(Microsecond, Some("UTC"))
array: PrimitiveArray<Timestamp(Nanosecond, None)>
[
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
2020-01-01T09:02:03.123456,
],
casted: Ok(PrimitiveArray<Timestamp(Microsecond, Some("UTC"))>
[
2020-01-01T09:02:03.123456+00:00,
2020-01-01T09:02:03.123456+00:00,
2020-01-01T09:02:03.123456+00:00,
2020-01-01T09:02:03.123456+00:00,
2020-01-01T09:02:03.123456+00:00,
2020-01-01T09:02:03.123456+00:00,
2020-01-01T09:02:03.123456+00:00,
2020-01-01T09:02:03.123456+00:00,
2020-01-01T09:02:03.123456+00:00,
2020-01-01T09:02:03.123456+00:00,
])
So the difference in behavior is because of a different timezone being applied. Both are using session timezone, but one of them is wrong. Either way, we cannot pin the failures on this PR.
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1954307910
##########
native/spark-expr/src/utils.rs:
##########
@@ -72,9 +72,6 @@ pub fn array_with_timezone(
Some(DataType::Timestamp(_, Some(_))) => {
timestamp_ntz_to_timestamp(array, timezone.as_str(), Some(timezone.as_str()))
}
- Some(DataType::Timestamp(_, None)) => {
Review Comment:
I would be surprised if it came in for fuzz tests since the blame has the line coming in with the experimental native scans PR. AFAIK, parquet_support no longer depends on utils.rs after this PR. I was also just removing this errant line.
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2656275607
> So it looks like this PR does not have a negative impact on `native_iceberg_compat`
Well that's good news!
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1953778478
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -618,174 +143,20 @@ fn cast_array(
Dictionary(_, _) => Arc::new(casted_dictionary.clone()),
_ => take(casted_dictionary.values().as_ref(), dict_array.keys(), None)?,
};
- return Ok(spark_cast_postprocess(casted_result, &from_type, to_type));
+ return Ok(casted_result);
}
_ => array,
};
let from_type = array.data_type();
- let eval_mode = parquet_options.eval_mode;
- let cast_result = match (from_type, to_type) {
- (Utf8, Boolean) => spark_cast_utf8_to_boolean::<i32>(&array, eval_mode),
- (LargeUtf8, Boolean) => spark_cast_utf8_to_boolean::<i64>(&array, eval_mode),
- (Utf8, Timestamp(_, _)) => {
- cast_string_to_timestamp(&array, to_type, eval_mode, &parquet_options.timezone)
- }
- (Utf8, Date32) => cast_string_to_date(&array, to_type, eval_mode),
- (Int64, Int32)
- | (Int64, Int16)
- | (Int64, Int8)
- | (Int32, Int16)
- | (Int32, Int8)
- | (Int16, Int8)
- if eval_mode != EvalMode::Try =>
- {
- spark_cast_int_to_int(&array, eval_mode, from_type, to_type)
- }
- (Utf8, Int8 | Int16 | Int32 | Int64) => {
- cast_string_to_int::<i32>(to_type, &array, eval_mode)
- }
- (LargeUtf8, Int8 | Int16 | Int32 | Int64) => {
- cast_string_to_int::<i64>(to_type, &array, eval_mode)
- }
- (Float64, Utf8) => spark_cast_float64_to_utf8::<i32>(&array, eval_mode),
- (Float64, LargeUtf8) => spark_cast_float64_to_utf8::<i64>(&array, eval_mode),
- (Float32, Utf8) => spark_cast_float32_to_utf8::<i32>(&array, eval_mode),
- (Float32, LargeUtf8) => spark_cast_float32_to_utf8::<i64>(&array, eval_mode),
- (Float32, Decimal128(precision, scale)) => {
- cast_float32_to_decimal128(&array, *precision, *scale, eval_mode)
- }
- (Float64, Decimal128(precision, scale)) => {
- cast_float64_to_decimal128(&array, *precision, *scale, eval_mode)
- }
- (Float32, Int8)
- | (Float32, Int16)
- | (Float32, Int32)
- | (Float32, Int64)
- | (Float64, Int8)
- | (Float64, Int16)
- | (Float64, Int32)
- | (Float64, Int64)
- | (Decimal128(_, _), Int8)
- | (Decimal128(_, _), Int16)
- | (Decimal128(_, _), Int32)
- | (Decimal128(_, _), Int64)
- if eval_mode != EvalMode::Try =>
- {
- spark_cast_nonintegral_numeric_to_integral(&array, eval_mode, from_type, to_type)
- }
- (Struct(_), Utf8) => Ok(casts_struct_to_string(array.as_struct(), parquet_options)?),
+ match (from_type, to_type) {
(Struct(_), Struct(_)) => Ok(cast_struct_to_struct(
array.as_struct(),
from_type,
to_type,
parquet_options,
)?),
- (UInt8 | UInt16 | UInt32 | UInt64, Int8 | Int16 | Int32 | Int64)
- if parquet_options.allow_cast_unsigned_ints =>
- {
- Ok(cast_with_options(&array, to_type, &PARQUET_OPTIONS)?)
- }
- _ if parquet_options.is_adapting_schema
- || is_datafusion_spark_compatible(
- from_type,
- to_type,
- parquet_options.allow_incompat,
- ) =>
- {
- // use DataFusion cast only when we know that it is compatible with Spark
- Ok(cast_with_options(&array, to_type, &PARQUET_OPTIONS)?)
- }
- _ => {
- // we should never reach this code because the Scala code should be checking
- // for supported cast operations and falling back to Spark for anything that
- // is not yet supported
- Err(SparkError::Internal(format!(
- "Native cast invoked for unsupported cast from {from_type:?} to {to_type:?}"
- )))
- }
- };
- Ok(spark_cast_postprocess(cast_result?, from_type, to_type))
-}
-
-/// Determines if DataFusion supports the given cast in a way that is
-/// compatible with Spark
-fn is_datafusion_spark_compatible(
- from_type: &DataType,
- to_type: &DataType,
- allow_incompat: bool,
-) -> bool {
- if from_type == to_type {
- return true;
- }
- match from_type {
- DataType::Boolean => matches!(
- to_type,
- DataType::Int8
- | DataType::Int16
- | DataType::Int32
- | DataType::Int64
- | DataType::Float32
- | DataType::Float64
- | DataType::Utf8
- ),
- DataType::Int8 | DataType::Int16 | DataType::Int32 | DataType::Int64 => {
- // note that the cast from Int32/Int64 -> Decimal128 here is actually
- // not compatible with Spark (no overflow checks) but we have tests that
- // rely on this cast working so we have to leave it here for now
- matches!(
- to_type,
- DataType::Boolean
- | DataType::Int8
- | DataType::Int16
- | DataType::Int32
- | DataType::Int64
- | DataType::Float32
- | DataType::Float64
- | DataType::Decimal128(_, _)
- | DataType::Utf8
- )
- }
- DataType::Float32 | DataType::Float64 => matches!(
- to_type,
- DataType::Boolean
- | DataType::Int8
- | DataType::Int16
- | DataType::Int32
- | DataType::Int64
- | DataType::Float32
- | DataType::Float64
- ),
- DataType::Decimal128(_, _) | DataType::Decimal256(_, _) => matches!(
- to_type,
- DataType::Int8
- | DataType::Int16
- | DataType::Int32
- | DataType::Int64
- | DataType::Float32
- | DataType::Float64
- | DataType::Decimal128(_, _)
- | DataType::Decimal256(_, _)
- | DataType::Utf8 // note that there can be formatting differences
- ),
- DataType::Utf8 if allow_incompat => matches!(
- to_type,
- DataType::Binary | DataType::Float32 | DataType::Float64 | DataType::Decimal128(_, _)
- ),
- DataType::Utf8 => matches!(to_type, DataType::Binary),
- DataType::Date32 => matches!(to_type, DataType::Utf8),
- DataType::Timestamp(_, _) => {
- matches!(
- to_type,
- DataType::Int64 | DataType::Date32 | DataType::Utf8 | DataType::Timestamp(_, _)
- )
- }
- DataType::Binary => {
- // note that this is not completely Spark compatible because
- // DataFusion only supports binary data containing valid UTF-8 strings
- matches!(to_type, DataType::Utf8)
- }
- _ => false,
+ _ => Ok(cast_with_options(&array, to_type, &PARQUET_OPTIONS)?),
Review Comment:
+1
##########
native/spark-expr/src/utils.rs:
##########
@@ -72,9 +72,6 @@ pub fn array_with_timezone(
Some(DataType::Timestamp(_, Some(_))) => {
timestamp_ntz_to_timestamp(array, timezone.as_str(), Some(timezone.as_str()))
}
- Some(DataType::Timestamp(_, None)) => {
Review Comment:
iirc this was introduced to fix cast issues caught by the fuzz tests which are not part of the ci. Can we run the fuzz tests again to verify that there are no regressions.
Ideally, parquet_support should _not_ depend on any functions in utils.rs.
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1951123998
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
Review Comment:
This was causing the initial time zone change.
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1953466766
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
Review Comment:
Yep I removed that case in utils.rs.
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1952625044
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
Review Comment:
Yeah, it shouldn't be called with that type:
https://github.com/apache/datafusion-comet/blob/f099e6e40aa18441c7882e5bffd9d6dfb10c6c19/native/spark-expr/src/utils.rs#L59
The condition that matches inside `array_with_timezone` was added by #1348...
https://github.com/apache/datafusion-comet/blob/f099e6e40aa18441c7882e5bffd9d6dfb10c6c19/native/spark-expr/src/utils.rs#L76
...and we likely didn't understand the blast radius at the time.
--
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] fix: Reduce cast.rs and utils.rs logic from parquet_support.rs for experimental native scans [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1955309371
##########
native/spark-expr/src/utils.rs:
##########
@@ -72,9 +72,6 @@ pub fn array_with_timezone(
Some(DataType::Timestamp(_, Some(_))) => {
timestamp_ntz_to_timestamp(array, timezone.as_str(), Some(timezone.as_str()))
}
- Some(DataType::Timestamp(_, None)) => {
Review Comment:
+1 for removing this as I see that this came in as a part of https://github.com/apache/datafusion-comet/pull/1318
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2656274826
> I had the impression that this PR had originally reduced the number of failures in native_iceberg_compat as well but that is no longer true after the cleanup. Is that correct?
Initially that was the case, then I got more aggressive with pruning parquet_support.
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1953416190
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
Review Comment:
Or what about updating the logic in `array_with_timezone` rather than fixing at this location?
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1952620440
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
+ _ => array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?,
Review Comment:
I'm slowly going through all of the behavior that we hoisted from cast.rs to parquet_support.rs, but yep I can file an issue.
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2651512371
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/1387?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 34.34%. Comparing base [(`f09f8af`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`6008a2e`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/6008a2e06c2e4149baf278c3ed4080de94bfe62b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 23 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1387 +/- ##
=============================================
- Coverage 56.12% 34.34% -21.78%
- Complexity 976 1064 +88
=============================================
Files 119 142 +23
Lines 11743 48698 +36955
Branches 2251 10648 +8397
=============================================
+ Hits 6591 16727 +10136
- Misses 4012 28608 +24596
- Partials 1140 3363 +2223
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/1387?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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1952625044
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
Review Comment:
Yeah, it shouldn't be called with that type:
https://github.com/apache/datafusion-comet/blob/f099e6e40aa18441c7882e5bffd9d6dfb10c6c19/native/spark-expr/src/utils.rs#L59
The condition that matches inside `array_with_timezone` was added by #1348 and we likely didn't understand the blast radius at the time.
--
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] fix: Remove cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2654633256
For context: when we were working in the experimental branch and added the `SchemaAdapter`, we were calling into cast.rs for type conversions. However, we started making changes in cast.rs to get more tests to pass for the experimental native scans. Not wanting to commingle logic and accidentally break cast behavior, we duplicated cast.rs and created parquet_support.rs to start making changes in. I now realize that duplicating cast.rs might have been a mistake, and have removed most of that logic from parquet_support.rs. This PR now defaults to DataFusion's cast logic unless we need specialized logic as in `cast_struct_to_struct`.
This change does not introduce any new failures for the experimental native scans, but that doesn't mean this approach is bug free. @andygrove is working on some fuzz testing that will help us have more confidence in the experimental native scans in the future. I propose we move forward with this design, and bring over (or call into) cast.rs logic as we discover issues with this design.
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2654730899
Updated the test failures in the original description. `native_iceberg_compat` really does not like this change.
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1953466766
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
Review Comment:
Yep I updated `array_with_timezone`.
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2655953229
I applied this PR to my working branch : https://github.com/parthchandra/datafusion-comet/commits/complex_test_failures_with_PR1387/
The branch contains all the changes in https://github.com/apache/datafusion-comet/pull/1376 and some additional fixes I have been working on. The results are:
native_datafusion: *** 18 TESTS FAILED ***
native_iceberg_compat: *** 27 TESTS FAILED ***
The differences are due to 5 tests related to
1) runs of `- fix: native Unsafe row accessors return incorrect results` which seems to fallback to Spark with native_datafusion (and so always succeeds).
2) 6 tests related to int96 fail in native_iceberg_compat. The failure might be related to the Spark configuration - `PARQUET_INT96_TIMESTAMP_CONVERSION i.e "spark.sql.parquet.int96TimestampConversion"`
Investigating this, but it is unlikely to be related to this PR.
So it looks like this PR does not have a negative impact on `native_iceberg_compat`
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2655443473
I had the impression that this PR had originally reduced the number of failures in native_iceberg_compat as well but that is no longer true after the cleanup. Is that correct?
--
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] fix: Reduce cast.rs and utils.rs logic from parquet_support.rs for experimental native scans [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2660476596
@kazuyukitanimura please go ahead and merge this.
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#issuecomment-2651252516
utils.rs's `array_with_timezone` mentions it doesn't support converting to NTZ, but we were calling it. I am gonna try an assertion in there and see if that breaks anything.
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1951749187
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
Review Comment:
This will skip all the matches inside `array_with_timezone`
```
match array.data_type() {
DataType::Timestamp(_, None) => {
...
match to_type {
...
Some(DataType::Timestamp(_, None)) => {
timestamp_ntz_to_timestamp(array, timezone.as_str(), None)
}
...
```
Is it intentional?
--
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] fix: Reduce timestamp issues in native_datafusion and native_icerberg_compat Parquet modes [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1952694138
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
Review Comment:
`utils.rs` timezone functions are intended for use by `cast` and specifically convert between Spark representation and Arrow representation. Changing them is likely to break some timezone related unit tests.
--
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] fix: Reduce cast.rs logic from parquet_support.rs for experimental native readers [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387#discussion_r1953416190
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -596,7 +595,10 @@ fn cast_array(
parquet_options: &SparkParquetOptions,
) -> DataFusionResult<ArrayRef> {
use DataType::*;
- let array = array_with_timezone(array, parquet_options.timezone.clone(), Some(to_type))?;
+ let array = match to_type {
+ Timestamp(_, None) => array, // array_with_timezone does not support to_type of NTZ.
Review Comment:
Or what about updating the case match in `array_with_timezone` rather than fixing at this location?
--
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] fix: Reduce cast.rs and utils.rs logic from parquet_support.rs for experimental native scans [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura merged PR #1387:
URL: https://github.com/apache/datafusion-comet/pull/1387
--
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