Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Customizable column types and validators #160

Merged
merged 4 commits into from
Feb 9, 2023

Conversation

melgenek
Copy link
Contributor

@melgenek melgenek commented Feb 7, 2023

Signed-off-by: Yevhenii Melnyk melnyk.yevhenii@gmail.com

Partially closes #36.

Rationale for the change

I'd like to introduce column type verification for Datafusion apache/datafusion#4499.

In order to let downstream users of sqllogictest-rs to define custom column types and validation strategies, I suggest introducing a pluggable column type. This way different AsynDB and DB implementations would be able to define column types beyond the original 'T','I','R' like CocroachDB does, or shrinking the set of columns to only having I like DuckDB suggests.

Changes

This pr introduces an interface ColumnType and a default implementation DefaultColumnType that is similar to the original Sqllites type, except that it accepts any letters and parses them as an "Any" type.

Another change is a customizable validator for columns in results. By default, the behaviour is the same as before: neither the number nor type of the columns is validated.

Changes visible to the client

  • AsyncDB and DB implementations would need to explicitly define an associated type ColumnType.
  • The run method would need to change the definition
from 
fn run(&mut self, sql: &str) -> Result<DBOutput> {...}

to 
fn run(&mut self, sql: &str) -> Result<DBOutput<Self::ColumnType>, FakeDBError> {...}
  • ColumnType::Any -> DefaultColumnType::Any
  • The way to call update_test_file would change from update_test_file(.., runner, ..) to runner.update_test_file(...)

Signed-off-by: Yevhenii Melnyk <melnyk.yevhenii@gmail.com>
Signed-off-by: Yevhenii Melnyk <melnyk.yevhenii@gmail.com>
Copy link
Member

@xxchan xxchan 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 very much for your recent contributions! I'm sorry that I havn't have a change to look at them. I will take a look soon. Also cc @skyzh do you have time to take a look? 🫣

Signed-off-by: Yevhenii Melnyk <melnyk.yevhenii@gmail.com>
@skyzh skyzh self-requested a review February 7, 2023 21:14
Copy link
Member

@skyzh skyzh 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 a lot for this great work!

Given this is a breaking change, we will need to release a minor version for the current main branch before merging this PR. I will do it today.

@skyzh skyzh enabled auto-merge (squash) February 9, 2023 15:32
@skyzh skyzh merged commit 2cbe58d into risinglightdb:main Feb 9, 2023
@melgenek
Copy link
Contributor Author

melgenek commented Feb 9, 2023

@skyzh Wow, thank you for so fast review!

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.

verify column in option is the same as provided input
3 participants