Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make DataFusion Core compile #3

Merged
merged 9 commits into from
Sep 17, 2021
Merged

Conversation

yjshen
Copy link
Collaborator

@yjshen yjshen commented Sep 16, 2021

No description provided.

@@ -0,0 +1,302 @@
// Licensed to the Apache Software Foundation (ASF) under one
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file is a copy of arrow/src/compute/kernels/cast_utils.rs.

Arrow2 currently only provide utf8_to_timestamp_ns_scalar with explicit timezone and fmt string, or utf8_to_naive_timestamp_ns_scalar with explicit fmt string. It's not possible to achieve identical semantic with the original arrow-rs string_to_timestamp_nanos in arrow2 since (as I quote jorge here)

it mixes timezone-aware, non-timezone aware, and local time in the same conversion. That IMO is not a “cast”: it is a “let me try anything to not crash irrespectively of its correctness”. For people that do rely on timezones, imo that is a foot gun that hides bugs. It also goes against the arrow spec that states that naive timestamps are not utc.

I added this back as a temporary workaround to make the build pass. We need to redesign the time-related API by providing users with two possible flavors: one with explicit timezone setting and the other default to local timezone, but both of the API variants should ask user for datetime format string. I'll file an issue on this later.

Copy link
Owner

@houqp houqp Sep 17, 2021

Choose a reason for hiding this comment

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

i think it's the right decision to move this logic into datafusion.

@@ -197,7 +197,7 @@ impl ExecutionPlan for SortPreservingMergeExec {
/// `SortKeyCursor::compare` can then be used to compare the sort key pointed to
/// by this row cursor, with that of another `SortKeyCursor`. A cursor stores
/// a row comparator for each other cursor that it is compared to.
struct SortKeyCursor<'a> {
struct SortKeyCursor {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ported apache/arrow-rs#542 back to arrow2, please merge the latest master into your ord branch. @houqp

@yjshen
Copy link
Collaborator Author

yjshen commented Sep 16, 2021

@houqp To make your local arrow2-merge branch compile and fail just like mine: Please pull parquet2 latest main as well as merge arrow2 latest main to the working branch.

I have this in my root cargo.toml now:

[patch.crates-io]
arrow2 = { path = "/Users/shenyijie/oss/arrow2" }
parquet2 = { path = "/Users/shenyijie/oss/parquet2"}

@houqp houqp merged commit 478f606 into houqp:arrow2-merge Sep 17, 2021
houqp pushed a commit that referenced this pull request Oct 1, 2021
* # This is a combination of 3 commits.
# This is the 1st commit message:

Add Display for Expr::BinaryExpr

# This is the commit message #2:

Update logical_plan/operators tests

# This is the commit message #3:

rebase and debug display for non binary expr

* Add Display for Expr::BinaryExpr

Update logical_plan/operators tests

rebase and debug display for non binary expr

Add Display for Expr::BinaryExpr

Update logical_plan/operators tests

Updating tests

Update aggregate display

Updating tests without aggregate

More tests

Working on agg/scalar functions

Fix binary_expr in create_name function and attendant tests

More tests

More tests

Doc tests

Rebase and update new tests

* Submodule update

* Restore submodule references from master

Co-authored-by: Andrew Lamb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants