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

chore(placeholder): update error message and add tests #9073

Merged
merged 4 commits into from
Jan 31, 2024

Conversation

appletreeisyellow
Copy link
Contributor

@appletreeisyellow appletreeisyellow commented Jan 30, 2024

Which issue does this PR close?

Relate to #8819

Rationale for this change

To help users use placeholder and parameter values

What changes are included in this PR?

No code change, only added tests

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the core Core datafusion crate label Jan 30, 2024
@github-actions github-actions bot added the logical-expr Logical plan and expressions label Jan 30, 2024
@appletreeisyellow appletreeisyellow changed the title test(placeholder): add positive test test(placeholder): update error message and add tests Jan 30, 2024
@appletreeisyellow appletreeisyellow changed the title test(placeholder): update error message and add tests chore(placeholder): update error message and add tests Jan 30, 2024
@github-actions github-actions bot added the sql label Jan 30, 2024
@appletreeisyellow appletreeisyellow marked this pull request as ready for review January 31, 2024 02:57
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @appletreeisyellow -- I think this code is an improvement in that it documents what the current behavior is.

However, I am not sure if this resolves #8819, because I am not sure what @simonvandel is trying to do.

I am trying to clarify here #8819 (comment)

@@ -251,3 +251,63 @@ async fn test_parameter_invalid_types() -> Result<()> {
);
Ok(())
}

#[tokio::test]
async fn test_parameter_empty_table_with_param_values() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given these tests aren't SQL but use the DataFrame API they probably belong in https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/dataframe instead

chore: add positive test and remove println
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @appletreeisyellow

@alamb alamb merged commit 7ab03e7 into apache:main Jan 31, 2024
22 checks passed
@appletreeisyellow appletreeisyellow deleted the chunchun/test-placeholder branch January 31, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core datafusion crate logical-expr Logical plan and expressions sql
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants