Skip to content

Simplify making information_schame tables#10420

Merged
alamb merged 6 commits intoapache:mainfrom
lewiszlw:simplify-information_schema-tables
May 8, 2024
Merged

Simplify making information_schame tables#10420
alamb merged 6 commits intoapache:mainfrom
lewiszlw:simplify-information_schema-tables

Conversation

@lewiszlw
Copy link
Copy Markdown
Member

@lewiszlw lewiszlw commented May 8, 2024

Which issue does this PR close?

Simplify making information_schame tables and avoid potential bug that someone forgets adding logic for new table here.

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 the core Core DataFusion crate label May 8, 2024
Comment thread datafusion/core/src/catalog/information_schema.rs Outdated
Copy link
Copy Markdown
Member

@Weijun-H Weijun-H left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @lewiszlw

Comment thread datafusion/core/src/catalog/information_schema.rs Outdated
async fn table(
&self,
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError> {
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.

Not related to this PR but do we need Result in return 🤔

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.

I think it is related to the fact this is in the implementation of the SchemaProvider trait

Copy link
Copy Markdown
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Thanks @lewiszlw please take care on case insensitive comment

@Weijun-H
Copy link
Copy Markdown
Member

Weijun-H commented May 8, 2024

I am surprised that this pr passed the tests, how about adding some tests for it?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label May 8, 2024
@lewiszlw
Copy link
Copy Markdown
Member Author

lewiszlw commented May 8, 2024

I am surprised that this pr passed the tests, how about adding some tests for it?

I added a test in slt.

Copy link
Copy Markdown
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 great to me -- thank you @lewiszlw

Also thank you @Weijun-H and @comphead (nice catch)

async fn table(
&self,
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError> {
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.

I think it is related to the fact this is in the implementation of the SchemaProvider trait

@alamb alamb merged commit 801dc17 into apache:main May 8, 2024
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
* Simplify making information_schame tables

* Simplify InformationSchemaProvider

* Remove lifetime annotation

* Replace if with match

* case insensitive

* Add test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants