-
Notifications
You must be signed in to change notification settings - Fork 161
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: add bft function testcases in new substrait testfile format #738
Conversation
The breaking changes check failure is not related to this commit. It pointed to changes in update rel. Once we rebase, it will pass. |
06e36de
to
83a1f51
Compare
We should get CI running first for the coverage code before adding these. That way we won't accidentally add broken things. Second, we should also add the automated tool that does the conversion so other people can use it if they have their own tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turned into a review of the BFT source testcases since the conversion process is working correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the count_star test file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The count_star() function name doesn't match any function in substrait functions. So, unable to add it as coverage breaks.
lower('aBc'::str) = 'abc'::str | ||
lower('abc'::str) = 'abc'::str | ||
lower(''::str) = ''::str | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
future: we are going to need to revisit all of the string cases once we add collation and character set. (For instance, the capital letter I in Turkish does not become i but a version without a dot.)
repeat(''::str, 2::i64) = ''::str | ||
|
||
# null_input: Examples with null as input | ||
repeat(null::str, 2::i64) = null::str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
later: add tests for null and negative counts
Raised a bft PR substrait-io/bft#97 for the script as well. |
I took care of review comments but had to rebase because of conflicting file. Below are the different cases which caused signature mismatches even for functions which have the right test cases, addressed almost all of them (4 is not yet fixed). Now I am left with around 250 odd test cases. Now, mostly I see failures which are actual signature mismatches or (4.)
|
raised PR #744 to fix issues with function lookup in coverage tool. |
b9a17c8
to
16ce9ed
Compare
4bbcfe9
to
cf8851f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still the question of extract with microsecond but since that's mirroring the existing test it's fine to investigate it later.
I raised a draft PR substrait-io/bft#98 to check how pipeline runs go with these testcases on bft. |
I checked locally by using the DRAFT PR for duckdb, snowflake, postgres and sqllite. Made sure there are no regressions on them. The previous patch had one offending test case for sqllite, removed that testcase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @srikrishnak and @EpsilonPrime ! Thanks for thorough update and review.
No description provided.