bugfix: fix tpcds_logical_q8 ambiguous name. #5335
Conversation
There was a problem hiding this comment.
This looks ok to me @jackwener -- but I think we should have some sort of test that passes with this change and fails without it. The rationale being that without such coverage this logic may get lost during a refactoring or some other code change and we wouldn't know because no test would fail
| let fields_without_qualifier = matches | ||
| .iter() | ||
| .filter(|f| f.qualifier.is_none()) | ||
| .collect::<Vec<_>>(); | ||
| if fields_without_qualifier.len() == 1 { | ||
| Ok(fields_without_qualifier[0]) |
There was a problem hiding this comment.
You might be able to write this using find instead
Something like
| let fields_without_qualifier = matches | |
| .iter() | |
| .filter(|f| f.qualifier.is_none()) | |
| .collect::<Vec<_>>(); | |
| if fields_without_qualifier.len() == 1 { | |
| Ok(fields_without_qualifier[0]) | |
| if let Some(fields_without_qualifier) = matches | |
| .iter() | |
| .find(|f| f.qualifier.is_none()) { | |
| Ok(fields_without_qualifier[0]) |
There was a problem hiding this comment.
When fields_without_qualifier size > 1, we should still return a Error.
So we shouldn't use find
Thank you @alamb , I add a new integration-test for this issue. |
|
Benchmark runs are scheduled for baseline = 554852e and contender = f6e49ac. f6e49ac is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* bugfix: fix tpcds_logical_q8 ambiguous name. * add integration-test
Which issue does this PR close?
Closes #5334.
Part of #4685.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
This PR fix tpcds_logical_q8.
Are there any user-facing changes?