-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[feat]:fast check has column #5328
[feat]:fast check has column #5328
Conversation
It seems that something went wrong with github's servers in the morning, so I reopen this request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @suxiaogang223
I am not sure using Result will ever be faster
If you think this will be faster, we can run the benchmark added in #5256
cargo bench --bench sql_planner
error should not be returned
Co-authored-by: Daniël Heres <danielheres@gmail.com>
Co-authored-by: Daniël Heres <danielheres@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this PR carefully, it looks like it has been polished.
Please fix the conflict, thank you @suxiaogang223
datafusion/sql/src/query.rs
Outdated
columns.into_iter().try_for_each::<_, Result<()>>(|c| { | ||
if !schema.has_column(&c) { | ||
missing_cols.push(c); | ||
} | ||
}); | ||
Ok(()) | ||
})?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
columns.into_iter().for_each(|c| {
if !schema.has_column(&c) {
missing_cols.push(c);
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @suxiaogang223 -- I agree with @jackwener this is looking great
I changed this PR's description to be I also took the liberty of merging from main to resolve the merge conflict in 8cd7f9b and fixing a new instance of |
Benchmark runs are scheduled for baseline = 1455b02 and contender = be6efbc. be6efbc is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Thank you very much, I have been a little slow to reply recently。I'm happy to work about datafusion。😄 |
we are happy to have the contribution 🤗 |
Which issue does this PR close?
part of #5309
Rationale for this change
What changes are included in this PR?
add
had_column
in DFSchema, and replacefield_from_column(l).is_ok()
byhas_column(l)
Are these changes tested?
yes
Are there any user-facing changes?
no