Jefffrey commented on code in PR #19672:
URL: https://github.com/apache/datafusion/pull/19672#discussion_r2670540562
##########
datafusion/sql/src/statement.rs:
##########
@@ -102,78 +103,72 @@ fn get_schema_name(schema_name: &SchemaName) -> String {
/// Construct `TableConstraint`(s) for the given columns by iterating over
/// `columns` and extracting individual inline constraint definitions.
fn calc_inline_constraints_from_columns(columns: &[ColumnDef]) ->
Vec<TableConstraint> {
- let mut constraints = vec![];
+ let mut constraints: Vec<TableConstraint> = vec![];
for column in columns {
for ast::ColumnOptionDef { name, option } in &column.options {
match option {
- ast::ColumnOption::Unique {
- is_primary: false,
- characteristics,
- } => constraints.push(TableConstraint::Unique {
- name: name.clone(),
- columns: vec![IndexColumn {
- column: OrderByExpr {
- expr: SQLExpr::Identifier(column.name.clone()),
- options: OrderByOptions {
- asc: None,
- nulls_first: None,
+ ast::ColumnOption::Unique(constraint) => {
Review Comment:
Should we destructure `constraint` here to make us more resilient to any
changes it has in the future perhaps? 🤔
(As in to ensure we catch any changes that can accept the syntax we accept)
##########
datafusion/sql/src/values.rs:
##########
@@ -31,6 +31,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
let SQLValues {
explicit_row: _,
rows,
+ value_keyword: _,
Review Comment:
What impact does ignoring this have? Does it change the syntax we accept?
##########
datafusion/sql/src/query.rs:
##########
@@ -170,6 +170,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
name: alias,
// Apply to all fields
columns: vec![],
+ explicit: true,
Review Comment:
Is this matching the previous behaviour we have? Does it change any syntax
we accept?
##########
datafusion/expr/src/expr.rs:
##########
@@ -1403,7 +1403,9 @@ pub struct PlannedReplaceSelectItem {
impl Display for PlannedReplaceSelectItem {
fn fmt(&self, f: &mut Formatter) -> fmt::Result {
write!(f, "REPLACE")?;
- write!(f, " ({})", display_comma_separated(&self.items))?;
+ for item in &self.items {
+ write!(f, " ({item})")?;
+ }
Review Comment:
I'm a bit curious on this change, it doesn't look right at first glance 🤔
##########
datafusion/sql/src/statement.rs:
##########
@@ -965,6 +963,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
has_table_keyword,
settings,
format_clause,
+ ..
Review Comment:
I'm a bit concerned on ignoring fields like this; it might make future
upgrades "easier" but it could mean our SQL syntax changes without us noticing
🤔
(Same goes for below and where we don't destructure inner struct items)
--
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]
