Skip to content

GenericDialect: support colon operator for JsonAccess - #2124

Merged
iffyio merged 1 commit into
apache:mainfrom
Samyak2:generic-json-access
Jan 16, 2026
Merged

GenericDialect: support colon operator for JsonAccess#2124
iffyio merged 1 commit into
apache:mainfrom
Samyak2:generic-json-access

Conversation

@Samyak2

Copy link
Copy Markdown
Contributor

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @Samyak2 -- makes sense to me

FYI @iffyio

@Samyak2

Copy link
Copy Markdown
ContributorAuthor

I will fix the CI in a bit - sorry did not notice that!

@Samyak2
Samyak2force-pushed the generic-json-access branch from 5b6007b to 4ae25fdCompareJanuary 7, 2026 13:38
@Samyak2

Copy link
Copy Markdown
ContributorAuthor

CI should pass now. There were some changes in main that I had not rebased on.

Comment threadtests/sqlparser_common.rs Outdated
Comment on lines +17979 to +17982
let dialects = TestedDialects::new(vec![
Box::new(GenericDialect {}),
Box::new(SnowflakeDialect {}),
]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this be all dialects instead? we don't seem to have special handling for generic and snowflake

@Samyak2Samyak2Jan 7, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We actually do have special handling for SnowflakeDialect and GenericDialect:

|| (dialect_of!(self is SnowflakeDialect | GenericDialect) && Token::Colon == *tok)

(this is in main currently, not a change in this PR)

From what I know, only Snowflake and Databricks support this syntax. So I can expand the test to include Databricks as well. But I don't think it would make sense to run this test on dialects that don't support this syntax. What do you think?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added Databricks to the list of dialects to test for.

Comment threadsrc/parser/mod.rs
} else {
Some(self.parse_expr()?)
// parse expr until we hit a colon (or any token with lower precedence)
Some(self.parse_subexpr(self.dialect.prec_value(Precedence::Colon))?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

for the changes to the subscript behavior, we don't seem to have any new tests to accompany them?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

These changes actually came from a failing test, but I will add some more tests to explicitly look for this behavior.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added some parse_array_subscript tests. I have verified that these catch the fixes made by these changes in subscript behavior.

@alamb

Copy link
Copy Markdown
Contributor

@Samyak2 any chance you can resolve the conflicts in this PR and address @iffyio 's comments? Then we can merge it in

- Port JsonAccess colon operator from Snowflake to Generic dialect
- This will be used in variant data type support in Datafusion
- see discussion in datafusion-contrib/datafusion-variant#2
@Samyak2

Copy link
Copy Markdown
ContributorAuthor

@Samyak2 any chance you can resolve the conflicts in this PR and address @iffyio 's comments? Then we can merge it in

Done!

@iffyioiffyio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @Samyak2

@iffyio
iffyio added this pull request to the merge queueJan 16, 2026
Merged via the queue into apache:main with commit 46f2234Jan 16, 2026
10 checks passed
ayman-sigma pushed a commit to sigmacomputing/sqlparser-rs that referenced this pull request Feb 3, 2026
Samyak2 added a commit to Samyak2/datafusion-sqlparser-rs that referenced this pull request Feb 6, 2026
- Fixesapache#2204
- Make map_field parsing aware of Colon token.
- This regression was likely introduced in apache#2124
- Added some tests for this.
- Not sure how the existing tests are passing, but I have verified
that the new tests catch this bug (they fail without the fix).
@Samyak2Samyak2 mentioned this pull request Feb 6, 2026
fmguerreiro pushed a commit to fmguerreiro/datafusion-sqlparser-rs that referenced this pull request Feb 20, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Samyak2@alamb@iffyio