LucaCappelletti94 commented on code in PR #2062:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2062#discussion_r2422754221
##########
src/parser/mod.rs:
##########
@@ -7929,22 +7929,22 @@ impl<'a> Parser<'a> {
loop {
if self.parse_keyword(Keyword::CONSTRAINT) {
let name = Some(self.parse_identifier()?);
- if let Some(option) = self.parse_optional_column_option()? {
+ if let Some(option) =
self.parse_optional_column_option(&col_name)? {
Review Comment:
I agree with that, but this philosophy of using the most strictly compatible
AST nodes in the different parts of the tree has already been broken several
times to reduce code duplication.
Here I am quite unsure whether the most appropriate solution would be to:
1) use the table constraint struct and duplicate the column ident
2) use the table constraint struct and leave its columns attribute empty
3) use a new struct to strictly represent the column option for this
constraint variant (for the same reasons of having structs instead of struct
variants)
I would personally be inclined with 1 or 2, but I understand the duplication
concerns of 1. Btw, is there any particular reason for the current clone-based
approach for idents instead of a copy and lifetime based one?
Lmk your opinion regarding how do you feel best proceeding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
