-
Notifications
You must be signed in to change notification settings - Fork 326
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
Merge Column_Indexes_Out_Of_Range
into Missing_Input_Columns
.
#6901
Merge Column_Indexes_Out_Of_Range
into Missing_Input_Columns
.
#6901
Conversation
- If a column index is out of range, a `Column_Indexes_Out_Of_Range` is | ||
raised as an error, unless `error_on_missing_columns` is set to | ||
`False`, in which case the problem is reported according to the | ||
`on_problems` setting. |
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 was mostly removing these without replacement, because IMO the upper comment is good enough for both index and name based scenarios:
- If a column in `columns` is not in the input table, a
`Missing_Input_Columns` is raised
If a "column" is not in the input table IMO works both when the column is represented by name, selector or the integer index.
@@ -1281,7 +1260,7 @@ type Table | |||
not possible to create a table without any columns. | |||
- If a given aggregate is not supported by the backend, | |||
`Unsupported_Database_Operation` is reported. | |||
- If a column index is out of range, a `Column_Indexes_Out_Of_Range` is |
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.
Here, an exception - the Text
case has some specific explanation and behaves slightly differently here. So I keep the special case for 'index' because it differs from Text here. I just update the comment to tell the current state.
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.
(this is due to handling differences caused by trying to parse the text as expression)
e52a4b3
to
e7a94c4
Compare
672f36f
to
4f60c8c
Compare
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.
LGTM - a nice simplification.
e7a94c4
to
9a5bc94
Compare
4f60c8c
to
a24cdf0
Compare
9a5bc94
to
7558c56
Compare
a24cdf0
to
af92a73
Compare
af92a73
to
fcae969
Compare
Pull Request Description
Implements #6869
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.