You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/10/31 03:50:46 UTC
[GitHub] [arrow-datafusion] tustvold opened a new pull request, #4039: Update to arrow 26
tustvold opened a new pull request, #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039
# 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.
-->
# 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.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4039: Update to arrow 26, change timezones
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#discussion_r1014343117
##########
datafusion/optimizer/tests/integration-test.rs:
##########
@@ -237,25 +235,26 @@ fn timestamp_nano_ts_none_predicates() -> Result<()> {
// a scan should have the now()... predicate folded to a single
// constant and compared to the column without a cast so it can be
// pushed down / pruned
- let expected = "Projection: test.col_int32\n Filter: test.col_ts_nano_utc < TimestampNanosecond(1666612093000000000, Some(\"UTC\"))\
- \n TableScan: test projection=[col_int32, col_ts_nano_none]";
+ let expected =
Review Comment:
Turns out this also fixes https://github.com/apache/arrow-datafusion/issues/3938 🎉
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4039: Update to arrow 26, change timezones
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1303986778
> sorry @alamb, I merged a PR that created a small conflict here
No worries -- I just fixed 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#discussion_r1014268129
##########
datafusion/core/tests/sql/mod.rs:
##########
@@ -1152,10 +1152,10 @@ pub fn make_timestamps() -> RecordBatch {
.map(|(i, _)| format!("Row {}", i))
.collect::<Vec<_>>();
- let arr_nanos = TimestampNanosecondArray::from_opt_vec(ts_nanos, None);
- let arr_micros = TimestampMicrosecondArray::from_opt_vec(ts_micros, None);
- let arr_millis = TimestampMillisecondArray::from_opt_vec(ts_millis, None);
- let arr_secs = TimestampSecondArray::from_opt_vec(ts_secs, None);
+ let arr_nanos = TimestampNanosecondArray::from(ts_nanos);
Review Comment:
❤️
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1302635513
Arrow 26 has been released -- https://github.com/apache/arrow-rs/issues/2953
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #4039: Update to arrow 26, change timezones
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1303986482
sorry @alamb, I merged a PR that created a small conflict here
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#discussion_r1014285308
##########
datafusion/expr/src/function.rs:
##########
@@ -219,7 +219,7 @@ pub fn return_type(
}
BuiltinScalarFunction::Now => Ok(DataType::Timestamp(
TimeUnit::Nanosecond,
- Some("UTC".to_owned()),
+ Some("+00:00".to_owned()),
Review Comment:
I basically applied this same pattern to a few other tests and now things looks great 👍
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ursabot commented on pull request #4039: Update to arrow 26, change timezones
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1304069122
Benchmark runs are scheduled for baseline = 4a67d0d2613d8c4b323e0dd5a6afe1eb1115c7ba and contender = dd081d64a2fba8574e63bdd0662c14aec5852b48. dd081d64a2fba8574e63bdd0662c14aec5852b48 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/b2e8be84c49a4bbfb7990e231c997487...88d20e146e1f44a4826847a763873d6a/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/c973c84b93a449278456729e863560d6...1d9789be6d464a3eb96500d3f6e2744c/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/392d724b6f90470ba9358b7ee009f93b...549ce895f1814795ad386a42a618c63c/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/16e63a8adb1c47438688e5780de2c7cc...8f5b05931d27417895ba90c0c98c6c68/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1296950856
Proposed fix https://github.com/apache/arrow-rs/pull/2991
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1297489136
I created a new release candidate for arrow
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1303890399
I am working on updating this PR and polishing it for merge
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1296908151
First thing I recommend we do is to file an arrow-rs ticket explaining the issue and then we can discuss a new RC based on that issue. Doing that now
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1296912078
Filed https://github.com/apache/arrow-rs/issues/2990 -- will work on that shortly
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
Dandandan commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1296756145
> @tustvold @Dandandan it seems like the limit is never applied for multi-column sorts when `lexsort_to_indices` is called. Probably due to the changes in [apache/arrow-rs#2929](https://github.com/apache/arrow-rs/pull/2929), where we previously created an array of `(&value_indices)[0..len]` but now just create it from `value_indices` directly without any limit (the single-column case is handled separately; and it still respects the limit).
Ah, great find. @tustvold might be worth to block the RC to fix this 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#discussion_r1009009343
##########
datafusion/expr/src/function.rs:
##########
@@ -219,7 +219,7 @@ pub fn return_type(
}
BuiltinScalarFunction::Now => Ok(DataType::Timestamp(
TimeUnit::Nanosecond,
- Some("UTC".to_owned()),
+ Some("+00:00".to_owned()),
Review Comment:
This change is necessary because by default (without the chrono-tz feature) arrow-rs doesn't know how to interpret this timezone. With the recent improvements to timezone handling, it now complains - (it prints "Unknown Timezone" when rending the timestamps)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1296529679
I'm getting failures in `test_sort_fetch_memory_calculation` which appears to have been added as part of #3593 by @isidentical. I'm having a hard time working out why it is failing, perhaps someone might be able help out?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#discussion_r1009009343
##########
datafusion/expr/src/function.rs:
##########
@@ -219,7 +219,7 @@ pub fn return_type(
}
BuiltinScalarFunction::Now => Ok(DataType::Timestamp(
TimeUnit::Nanosecond,
- Some("UTC".to_owned()),
+ Some("+00:00".to_owned()),
Review Comment:
This change is necessary because by default (without the chrono-tz feature) arrow-rs doesn't know how to interpret this timezone. With the recent improvements to timezone handling, it now complains - (it prints "Unknown Timezone" when rending the timestamps). _TBC this is an improvement over silently ignoring the timezone which it did before_
FYI @waitingkuo
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove merged pull request #4039: Update to arrow 26, change timezones
Posted by GitBox <gi...@apache.org>.
andygrove merged PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
andygrove commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1302686391
I plan on creating DataFusion 14.0.0 rc1 once this is merged.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
Dandandan commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1296608994
> I'm getting failures in `test_sort_fetch_memory_calculation` which appears to have been added as part of #3593 by @isidentical. I'm having a hard time working out why it is failing, perhaps someone might be able help out?
Somehow the implementation spilled memory, because it believes memory usage exceeded the limit. Maybe some change happened in arrow 26 not shrinking the buffer smaller somewhere?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] isidentical commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
isidentical commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1296752289
@tustvold @Dandandan it seems like the limit is never applied for multi-column sorts when `lexsort_to_indices` is called. Probably due to the changes in https://github.com/apache/arrow-rs/pull/2929, where we previously created an array of `(&value_indices)[0..len]` but now just create it from `value_indices` directly without any limit (the single-column case is handled separately; and it still respects the limit).
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#discussion_r1009009343
##########
datafusion/expr/src/function.rs:
##########
@@ -219,7 +219,7 @@ pub fn return_type(
}
BuiltinScalarFunction::Now => Ok(DataType::Timestamp(
TimeUnit::Nanosecond,
- Some("UTC".to_owned()),
+ Some("+00:00".to_owned()),
Review Comment:
This change is necessary because by default (without the chrono-tz feature) arrow-rs doesn't know how to interpret this timezone. With the recent improvements to timezone handling, it now complains - (it prints "Unknown Timezone" when rending the timestamps)
FYI @waitingkuo
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on pull request #4039: Update to arrow 26
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1296769875
Aah nice find will workaround.
Given it can easily be worked around by slicing the returned array I don't think we need to block the RC
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4039: Update to arrow 26, change timezones
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4039:
URL: https://github.com/apache/arrow-datafusion/pull/4039#issuecomment-1304157530
🚀 FYI @Ted-Jiang
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org