-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Add loop()
table function
#63452
Add loop()
table function
#63452
Conversation
This is an automated comment for commit 0f952cc with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
Hi, @sarielwxm. Thank you for your contribution to Clickhouse! I will be glad to review your PR. Firstly, I'd like to ask you to add the example output after execution of this table function that demonstrates its typical behaviour to the documentation. Secondly, could you please write a fast test that checks desired behaviour of the function? After these steps I will be able to continue the reviewing process |
Sure. I have already tested it locally. But I have a problem because this table function implements an infinite loop over the query results and it seems difficult to represent the example output. Could you please give me some suggestions? Much appreciated! |
Could you use LIMIT operator to bound the output of the loop function? |
Got it. It's useful. |
tests/queries/0_stateless/02414_all_new_table_functions_must_be_documented.sql
Outdated
Show resolved
Hide resolved
❌ Click here to open a full report on a separate page - you can find a report, why some CI tests failed. Tests that you added shouldn't be flaky. But there are some flaky tests in our repository (tests that fail in master CI, and they can fail in your PR as well). You can find them at https://aretestsgreenyet.com. If you see that a test fail both in your PR and master, you can forget about it more or less. But in our case, the new test is flaky that's why it should be fixed before PR merge. Everything else looks good to me |
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 is need to fix the flaky test
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.
LGTM. There are some issues with CI, I will try to resolve them by myself
2520e5c
Fixes #52765
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added a new table function
loop
to support returning query results in an infinite loop.Documentation entry for user-facing changes
Modify your CI run
NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step
Include tests (required builds will be added automatically):
Exclude tests:
Extra options:
Only specified batches in multi-batch jobs: