Skip to content

Conversation

@Samyak2
Copy link

@Samyak2 Samyak2 commented Dec 4, 2025

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.

Thank you @Samyak2 -- makes sense to me

FYI @iffyio

@Samyak2
Copy link
Author

Samyak2 commented Jan 6, 2026

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

@Samyak2 Samyak2 force-pushed the generic-json-access branch from 5b6007b to 4ae25fd Compare January 7, 2026 13:38
@Samyak2
Copy link
Author

Samyak2 commented Jan 7, 2026

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

Comment on lines 17979 to 18077
let dialects = TestedDialects::new(vec![
Box::new(GenericDialect {}),
Box::new(SnowflakeDialect {}),
]);
Copy link
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

Copy link
Author

@Samyak2 Samyak2 Jan 7, 2026

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
Author

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.

} 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
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
Author

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
Author

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
Contributor

alamb commented Jan 14, 2026

@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 Samyak2 force-pushed the generic-json-access branch from 4ae25fd to f365029 Compare January 15, 2026 18:58
@Samyak2
Copy link
Author

Samyak2 commented Jan 15, 2026

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

Done!

Sign up for free to 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