Skip to content

Conversation

@Standing-Man
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions core Core DataFusion crate labels Jan 7, 2026
@Standing-Man
Copy link
Contributor Author

Standing-Man commented Jan 7, 2026

@Jefffrey and @comphead PTAL when you have time. 😸

@Jefffrey
Copy link
Contributor

Jefffrey commented Jan 7, 2026

Could we address the CI failures? Looks like some tests are failing

Signed-off-by: StandingMan <jmtangcs@gmail.com>
Signed-off-by: StandingMan <jmtangcs@gmail.com>
Signed-off-by: StandingMan <jmtangcs@gmail.com>
@Standing-Man
Copy link
Contributor Author

@Jefffrey ready to review.

@alamb
Copy link
Contributor

alamb commented Jan 7, 2026

I was literally about to sit down and have codex try to do this -- thank you @Standing-Man -- taking a look now

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @Standing-Man -- thank you. THe only question I have is about the change of API to use Boxed results

Comment on lines 1406 to 1408
write!(f, " ({})", display_comma_separated(&self.items))?;
for item in &self.items {
write!(f, " ({item})")?;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit curious on this change, it doesn't look right at first glance 🤔

Copy link
Contributor Author

@Standing-Man Standing-Man Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you take a second look, you’ll see that the display_comma_separated method is a private helper used to format ReplaceSelectItem. 😄

https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/ast/query.rs#L960-L966

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we put two replace items in this test:

Some(PlannedReplaceSelectItem {
items: vec![ReplaceSelectElement {
expr: ast::Expr::Identifier(Ident::from("c1")),
column_name: Ident::from("a1"),
as_keyword: false
}],
planned_expressions: vec![]
}),

This new code will format the display like so:

* REPLACE (c1 a1) (c1 a1)

Which doesn't seem quite right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. the correct format display like:

* REPLACE (c1 a1, c1 a1)

i will update on next commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just reuse this function by removing its cfg guard:

#[cfg(not(feature = "sql"))]
pub fn display_comma_separated<T>(slice: &[T]) -> String
where
T: Display,
{
use itertools::Itertools;
slice.iter().map(|v| format!("{v}")).join(", ")
}

Signed-off-by: StandingMan <jmtangcs@gmail.com>
@Standing-Man Standing-Man requested review from Jefffrey and alamb January 8, 2026 05:54
Signed-off-by: StandingMan <jmtangcs@gmail.com>
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: StandingMan <jmtangcs@gmail.com>
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very tempting to not destructure the nested fields/enums because it makes the code less verbose. However, by doing so we miss fields that might be introduced in newer versions. These fields usually correspond to supporting more syntax. If we are unaware of this syntax, it means the SQL dialect we have will start accepting more and more syntax even if we don't do anything with the syntax.

I admit it might be painful and verbose to do it that way but I find its a very good way to use Rust's characteristics to help us catch this issues at compile time, instead of at runtime.

Comment on lines 1406 to 1408
write!(f, " ({})", display_comma_separated(&self.items))?;
for item in &self.items {
write!(f, " ({item})")?;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just reuse this function by removing its cfg guard:

#[cfg(not(feature = "sql"))]
pub fn display_comma_separated<T>(slice: &[T]) -> String
where
T: Display,
{
use itertools::Itertools;
slice.iter().map(|v| format!("{v}")).join(", ")
}

})),
ast::ColumnOption::PrimaryKey(PrimaryKeyConstraint {
characteristics,
..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're still ignoring fields here with the usage of ..

func_desc,
..
} => {
Statement::DropFunction(func) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be destructuring this like so

Statement::DropFunction(DropFunction {
    if_exists,
    func_desc,
    drop_behaviour,
})

characteristics,
} => constraints.push(TableConstraint::Unique {
ast::ColumnOption::Unique(UniqueConstraint {
characteristics, ..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be destructuring this not ignoring the fields with ..

}),
ast::ColumnOption::Check(expr) => {
constraints.push(TableConstraint::Check {
..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be destructuring this not ignoring the fields with ..

characteristics: *characteristics,
}))
}
ast::ColumnOption::Check(CheckConstraint { name, expr, .. }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be destructuring this not ignoring the fields with ..

or,
limit,
} => {
..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be destructuring this not ignoring the fields with ..

from,
order_by,
limit,
..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be destructuring this not ignoring the fields with ..

@Jefffrey
Copy link
Contributor

Jefffrey commented Jan 9, 2026

@Standing-Man could you please refrain from marking the comments as resolved? I see quite of few of my recent comments were marked as resolved even though no action was taken. Can you leave it to us to determine if a conversation is resolved, it makes it easier for us to review.

Signed-off-by: StandingMan <jmtangcs@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bump sqlparser from 0.59.0 to 0.60.0

3 participants