You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "logan-keede (via GitHub)" <gi...@apache.org> on 2025/01/25 21:38:30 UTC
[PR] refactor aggregate [datafusion]
logan-keede opened a new pull request, #14301:
URL: https://github.com/apache/datafusion/pull/14301
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #13723
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Better Readability and Navigation.
## What changes are included in this PR?
Refactor of `aggregate.slt` to multiple files,
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
`cargo test --test sqllogictests -- --complete` runs without failure, using patch mentioned in #14254
## Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor aggregate [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614315029
Main thing I am worried about is that this pr seems too large, it seems hard to ensure all exists testcases are moved rightly. Maybe we can push it forward more incrementally? I have some thoughts about it:
- Only extract the common part(mainly table creatings, e.g. `aggregate_test_100`, `d_table`, `test`...), and the files structure after extracting may be:
```text
-- aggregate
-- init_data.slt.part
-- aggregate.slt
```
- Sort the cases in `aggregate.slt` using comments firstly before moving to a specific `slt`, and think a way to ensure no cases are lost(I can help this step). It may be like:
```text
aggregate.slt
#######
# Count tests
#######
...
#######
# Sum tests
#######
...
#######
# Min/Max tests
#######
...
```
- Move and split the testcases to specific `slt`s.
```text
-- aggregate
-- init_data.slt.part
-- aggregate.slt (cases difficult to classify, or still waiting to be moved in following prs)
-- count.slt
-- sum.slt
-- min/max.slt
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on code in PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#discussion_r1929735407
##########
datafusion/sqllogictest/test_files/aggregate/string.slt:
##########
Review Comment:
🤔 just a bit worried that, when the splitting approach becomes complex, if it will be confusing about deciding which file should we place the new testcases into
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614412807
> Thanks for the review.
>
> > It seems workable too, but actullay a bit inconvenient to check if large changes in one pr?
>
> I meant to divide it into multiple PRs not commits.
>
> I will be opening a new pr with just addition of init.slt.aprt and supplement.slt shortly.
Ok, thanks @logan-keede
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor aggregate [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614287146
nit: I think PR title should reflect this is sqllogictest refactor to be more descriptive
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregation.slt` [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on code in PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#discussion_r1929703073
##########
datafusion/sqllogictest/test_files/aggregate/string.slt:
##########
Review Comment:
Seems switching to use data type to split rather than function?
##########
datafusion/sqllogictest/test_files/aggregate/aggregate.slt:
##########
@@ -0,0 +1,3053 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+
+# http://www.apache.org/licenses/LICENSE-2.0
+
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+#######
+# Setup test data table
+#######
+include ./init.slt.part
+
+# FIX: custom absolute values
+# csv_query_avg_multi_batch
+
+# csv_query_avg
+query R
+SELECT avg(c12) FROM aggregate_test_100
+----
+0.508972509913
+
+# csv_query_bit_and
+query IIIII
+SELECT bit_and(c5), bit_and(c6), bit_and(c7), bit_and(c8), bit_and(c9) FROM aggregate_test_100
+----
+0 0 0 0 0
+
+# csv_query_bit_and_distinct
+query IIIII
+SELECT bit_and(distinct c5), bit_and(distinct c6), bit_and(distinct c7), bit_and(distinct c8), bit_and(distinct c9) FROM aggregate_test_100
Review Comment:
It seems a part of `bit_and_or_xor`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor aggregate [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614315536
> Main thing I am worried about is that this pr seems too large, it seems hard to ensure all exists testcases are moved rightly. Maybe we can push it forward more incrementally? I have some thoughts about it:
>
> * Only extract the common part(mainly table creatings, e.g. `aggregate_test_100`, `d_table`, `test`...), and the files structure after extracting may be:
>
>
> ```
> -- aggregate
> -- init_data.slt.part
> -- aggregate.slt
> ```
>
> * Sort the cases in `aggregate.slt` using comments firstly before moving to a specific `slt`, and think a way to ensure no cases are lost(I can help this step). It may be like:
>
>
> ```
> aggregate.slt
>
> #######
> # Count tests
> #######
> ...
>
> #######
> # Sum tests
> #######
> ...
>
> #######
> # Min/Max tests
> #######
> ...
> ```
>
> * Move and split the testcases to specific `slt`s.
>
>
> ```
> -- aggregate
> -- init_data.slt.part
> -- aggregate.slt (cases difficult to classify, or still waiting to be moved in following prs)
> -- count.slt
> -- sum.slt
> -- min/max.slt
> ...
> ```
And how about we just start from the first step in this pr? And continue pushing it forward in following prs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor aggregate [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614197123
Seems still some failed tests exist, like:
```
External error: query failed: DataFusion error: Error during planning: table 'datafusion.public.float_table' not found
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614351520
> I think an alternative to this can be to divide this PR into increments and keep a aggregate_supplement.slt file and initially it will be same as aggregate.slt but as we move out queries we subtract them from the supplement file only. This way even if we there is some query that is left in supplement we can just move it later when it is discovered by using reference from original. (We can always find it in commit history but that's a bit inconvenient
@logan-keede I think it maybe nice to incrementally split cases like that.
Maybe we can start from the commonest ones(count, sum, avg, min/max and so on...), the safe way may be one pr just involve one function of them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor aggregate [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614108205
cc @Rachelint @Omega359
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614327958
agreed, I think if we are going to do this it might be better to open a new PR and start anew.
> * Sort the cases in `aggregate.slt` using comments firstly before moving to a specific `slt`, and think a way to ensure no cases are lost(I can help this step). It may be like:
a brutish way to solve this might be to keep `aggregate.slt` as a whole even after refactoring, it hardly adds a second to tests(we can add a redundancy warning to avoid misunderstanding), this way we have better navigation but also original testcases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614334330
> * Sort the cases in `aggregate.slt` using comments firstly before moving to a specific `slt`, and think a way to ensure no cases are lost(I can help this step). It may be like:
I think an alternative to this can be to divide this PR into increments and keep a `aggregate_supplement.slt` file and initially it will be same as `aggregate.slt` but as we move out queries we subtract them from the supplement file only. This way even if we there is some query that is left in `supplement` we can just move it later when it is discovered by using reference from original. (We can always find it in commit history but that's a bit inconvenient)
dejavu
> It seems a part of `bit_and_or_xor`?
I say this because this sorting might become a huge PR by itself, this approach divides this sorting into increments with PRs.
This is kind of what I am doing but in a more incremental manner with added redundancy.
@Rachelint Let me know if you think this is a bad Idea. I will open a new PR once we are on the same page.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede closed pull request #14301: Sort out testcases in `aggregate.slt`
URL: https://github.com/apache/datafusion/pull/14301
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614356225
> @logan-keede I think it nice to incrementally split cases like that.
Maybe we can start from the commonest ones(count, sum, avg, min/max and so on...), and the safe way may be one pr just involve one function of them?
And in this pr, we just extract the common table creating sql, and rename the `aggregate.slt` to `aggregate_supplement.slt`? After refactoring, it may be like:
```
-- aggregate
-- init_data.slt.part
-- aggregate_supplement.slt
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614410525
> > I think an alternative to this can be to divide this PR into increments and keep a aggregate_supplement.slt file and initially it will be same as aggregate.slt but as we move out queries we subtract them from the supplement file only. This way even if we there is some query that is left in supplement we can just move it later when it is discovered by using reference from original. (We can always find it in commit history but that's a bit inconvenient
>
> @logan-keede it is said that still one pr, but we perform `extract and subtract` for a specific function in one commit, and then we check it between commits?
>
> It seems workable too, but actullay a bit inconvenient to check if large changes in one pr?
>
> And for the extract and subtract approach, I think it seems really good.
Or I misunderstand? We will split it into multiple prs and we perform `extract and subtract` for a specific function in one pr? I think we may be on the same page if it means that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on code in PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#discussion_r1929767728
##########
datafusion/sqllogictest/test_files/aggregate/string.slt:
##########
Review Comment:
`aggregate.slt` already had loose subsections for eg. most of the tests in `date_time.slt` are made to work together(because of mix of different functions) + there is a section for postgres dialect only somewhere in original file. I doubt there would be much confusing tests left after that and we can just put them in both files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregate.slt` [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614410968
Thanks for the review.
> It seems workable too, but actullay a bit inconvenient to check if large changes in one pr?
I meant to divide it into multiple PRs not commits.
I will be opening a new pr with just addition of init.slt.aprt and supplement.slt shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor aggregate [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#issuecomment-2614270972
> Thanks @logan-keede
>
> I see still some failed tests exist, like:
>
> ```
> External error: query failed: DataFusion error: Error during planning: table 'datafusion.public.float_table' not found
> ```
fixed!
thanks for your patience
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Sort out testcases in `aggregation.slt` [datafusion]
Posted by "logan-keede (via GitHub)" <gi...@apache.org>.
logan-keede commented on code in PR #14301:
URL: https://github.com/apache/datafusion/pull/14301#discussion_r1929718734
##########
datafusion/sqllogictest/test_files/aggregate/string.slt:
##########
Review Comment:
function name is `STRING_AGG` maybe we should switch that. I think it might not be bad to have some split based on datatype like `date_time.slt` (present in this PR) even if we decide to make them redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org