Jefffrey commented on code in PR #19672:
URL: https://github.com/apache/datafusion/pull/19672#discussion_r2670979124
##########
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:
If we put two replace items in this test:
https://github.com/apache/datafusion/blob/102caeb2261c5ae006c201546cf74769d80ceff8/datafusion/expr/src/expr.rs#L3861-L3868
This new code will format the display like so:
```sql
* REPLACE (c1 a1) (c1 a1)
```
Which doesn't seem quite right?
##########
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:
This doesn't answer my question on if it matches our previous behaviour.
##########
datafusion/sql/src/values.rs:
##########
@@ -31,6 +31,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
let SQLValues {
explicit_row: _,
rows,
+ value_keyword: _,
Review Comment:
So it does change the syntax we accept? We should guard against this to
ensure we maintain our previous behaviour
##########
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:
I wasn't asking about `TableConstraint`. My point is having
`ast::ColumnOption::Unique(constraint)` means that new fields can be added to
`constraint` and there is no compile time detection of this, meaning new syntax
could slip in without us noticing
--
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]
