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

Fix DataFusion test and try to make ballista compile #4

Merged
merged 13 commits into from
Sep 18, 2021

Conversation

yjshen
Copy link
Collaborator

@yjshen yjshen commented Sep 17, 2021

No description provided.

@@ -236,7 +236,7 @@ mod tests {
#[test]
fn invalid_cast() {
// Ensure a useful error happens at plan time if invalid casts are used
let schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]);
let schema = Schema::new(vec![Field::new("a", DataType::Null, false)]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Int32 -> LargeBinary cast is valid in arrow2, therefore I change to another unacceptable case Null here

@@ -965,7 +965,7 @@ mod tests {
options: Default::default(),
},
PhysicalSortExpr {
expr: col("c7", &schema).unwrap(),
expr: col("c12", &schema).unwrap(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The original c7 is not distinguishable enough, i.e. same value exists many times.

Copy link

Choose a reason for hiding this comment

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

👍 The same thing happens in arrow 6.0 (as pointed out by @houqp ) -- more details here: https://github.com/apache/arrow-datafusion/pull/984/files#r705557467

"| e | 0.01479305307777301 | 0.9965400387585364 | 0.48600669271341534 | 10.206140546981722 | 21 | 21 |",
"| c | 0.0494924465469434 | 0.991517828651004 | 0.6600456536439785 | 13.860958726523547 | 21 | 21 |",
"| d | 0.061029375346466685 | 0.9748360509016578 | 0.48855379387549835 | 8.79396828975897 | 18 | 18 |",
"| e | 0.01479305307777301 | 0.9965400387585364 | 0.48600669271341557 | 10.206140546981727 | 21 | 21 |",
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 think this is due to float nature of inaccuracy, therefore acceptable.

@yjshen yjshen changed the title WIP: to make ballista compile Fix test and try to make ballista compile Sep 17, 2021
@yjshen yjshen changed the title Fix test and try to make ballista compile Fix DataFusion test and try to make ballista compile Sep 17, 2021
@@ -1394,7 +1394,7 @@ mod tests {
let expr = col("b1").not().eq(lit(true));
let p = PruningPredicate::try_new(&expr, schema).unwrap();
let result = p.prune(&statistics).unwrap();
assert_eq!(result, vec![true, false, false, true, true]);
assert_eq!(result, vec![true, true, false, true, true]);
Copy link
Owner

Choose a reason for hiding this comment

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

interesting, looks like arrow2 fixed a bug that exists in arrow-rs?

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 didn't get too much background on this, since this is a test against pruning of a != true, I think the current behavior is expected

@houqp houqp merged commit caf5b22 into houqp:arrow2-merge Sep 18, 2021
houqp pushed a commit that referenced this pull request Sep 19, 2021
* wip

* more

* Make scalar.rs compile

* Fix various compilation error due to API difference

* Make datafusion core compile

* fmt

* wip

* wip: compile ballista

* Pass all datafusion tests

* Compile ballista
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.

3 participants