Skip to content

fix: correct expected error in test #5224

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.

Sign up for GitHub

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

Jump to bottom
Merged
jackwener merged 3 commits into from
Feb 10, 2023
Merged

fix: correct expected error in test #5224

jackwener merged 3 commits into from
Feb 10, 2023

Conversation

@jackwener
Copy link
Member

@jackwener jackwener commented Feb 9, 2023 •
edited
Loading

Which issue does this PR close?

Closes #5217.
Part of #4685

Rationale for this change

What changes are included in this PR?

No need, current tests can cover this problem, but it was hidden by skip_failed_rules.

set pub skip_failed_rules: bool, default = false

Before this PR, cast_string_to_time and in_list_types_struct_literal will fail.
After this PR, they will success.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Feb 9, 2023
@jackwener
Copy link
Member Author

jackwener commented Feb 9, 2023

cc @HaoYang670

@jackwener jackwener changed the title fix: fix expect error in test cast_string_to_time fix: correct expected error in test Feb 9, 2023
@jackwener
Copy link
Member Author

jackwener commented Feb 9, 2023

This PR must be with disable skip config

@jackwener jackwener assigned jackwener and unassigned jackwener Feb 9, 2023
@jackwener jackwener marked this pull request as draft February 9, 2023 16:29
@HaoYang670
Copy link
Contributor

HaoYang670 commented Feb 10, 2023 •
edited
Loading

@jackwener If the error returned from type_coercion is the expected behavior, we can fix these when fixing #4615
@jackwener, could you try something like this to set skip_failed_rules to false in the test? I tested it on my desktop and it can work.

async fn cast_string_to_time() {
    let config = SessionConfig::new().set("datafusion.optimizer.skip_failed_rules", ScalarValue::Boolean(Some(false)));
    let ctx = SessionContext::with_config(config);

@jackwener
Copy link
Member Author

jackwener commented Feb 10, 2023 •
edited
Loading

@jackwener If the error returned from type_coercion is the expected behavior, we can fix these when fixing #4615 @jackwener, could you try something like this to set skip_failed_rules to false in the test? I tested it on my desktop and it can work.

async fn cast_string_to_time() {
    let config = SessionConfig::new().set("datafusion.optimizer.skip_failed_rules", ScalarValue::Boolean(Some(false)));
    let ctx = SessionContext::with_config(config);

It's great idea. thank you! @HaoYang670

@jackwener jackwener marked this pull request as ready for review February 10, 2023 06:07
HaoYang670
HaoYang670 reviewed Feb 10, 2023
View reviewed changes
Copy link
Contributor

@HaoYang670 HaoYang670 left a comment

Choose a reason for hiding this comment

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

LGTM.

datafusion/core/src/physical_plan/planner.rs
r#"type_coercion
caused by
Internal error: Optimizer rule 'type_coercion' failed due to unexpected error: Error during planning: Can not find compatible types to compare Boolean with [Struct([Field { name: "foo", data_type: Boolean, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }]), Utf8]. This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker"#
);
Copy link
Contributor

@HaoYang670 HaoYang670 Feb 10, 2023

Choose a reason for hiding this comment

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

We should update this error message somehow, because it is a type error, not a bug in Datafusion.
We could do this in the following PRs.

Copy link
Member Author

@jackwener jackwener Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agree with you.
I think we can do it in next PR.

datafusion/core/tests/sql/timestamp.rs
"Arrow error: Cast error: Cannot cast string 'not a time' to value of Time64(Nanosecond) type"
"simplify_expressions\ncaused by\nInternal error: Optimizer rule 'simplify_expressions' failed due to unexpected error: \
Arrow error: Cast error: Cannot cast string 'not a time' to value of Time64(Nanosecond) type. \
This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker"
Copy link
Contributor

@HaoYang670 HaoYang670 Feb 10, 2023

Choose a reason for hiding this comment

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

Ditto. We should find a way to update the error message when the error is expected. However, this is not a big deal now, because the default value of skip_failed_rules is false and customers won't see this error message.

datafusion/core/tests/sql/timestamp.rs
assert_eq!(
result.err().unwrap().to_string(),
"Arrow error: Cast error: Cannot cast string '24:01:02' to value of Time64(Nanosecond) type"
"simplify_expressions\ncaused by\nInternal error: Optimizer rule 'simplify_expressions' failed due to unexpected error: \
Copy link
Contributor

@HaoYang670 HaoYang670 Feb 10, 2023

Choose a reason for hiding this comment

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

Ditto.

@jackwener jackwener merged commit 8a262c3 into apache:master Feb 10, 2023
@jackwener jackwener deleted the fix_comment branch February 10, 2023 08:47
@ursabot
Copy link

ursabot commented Feb 10, 2023

Benchmark runs are scheduled for baseline = a1b6f50 and contender = 8a262c3. 8a262c3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@HaoYang670 HaoYang670 HaoYang670 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

core Core DataFusion crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The test in_list_types_struct_literal fails when setting skip_failed_rules as false

3 participants

@jackwener @HaoYang670 @ursabot