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

Fix analyzer with positional arguments in distributed query #62362

Merged
merged 7 commits into from
Apr 10, 2024

Conversation

ucasfl
Copy link
Collaborator

@ucasfl ucasfl commented Apr 6, 2024

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Resolve positional arguments only on the initiator node. Closes #62289.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/


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):

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with Analyzer
  • Add your option here

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • Add your option here

Extra options:

  • do not test (only style check)
  • disable merge-commit (no merge from master before tests)
  • disable CI cache (job reuse)

Only specified batches in multi-batch jobs:

  • 1
  • 2
  • 3
  • 4

@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 6, 2024
@robot-clickhouse-ci-1
Copy link
Contributor

robot-clickhouse-ci-1 commented Apr 6, 2024

This is an automated comment for commit eeeb8f3 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check nameDescriptionStatus
A SyncThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS⏳ pending
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR⏳ pending
Mergeable CheckChecks if all other necessary checks are successful❌ failure
Successful checks
Check nameDescriptionStatus
Fast testNormally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here✅ success
PR CheckThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success

Copy link
Member

@novikd novikd left a comment

Choose a reason for hiding this comment

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

In general LGTM, but I want to move condition into replaceNodesWithPositionalArguments to avoid mistakes in the future.

src/Analyzer/Passes/QueryAnalysisPass.cpp Outdated Show resolved Hide resolved
@novikd novikd self-assigned this Apr 8, 2024
@novikd novikd added analyzer Issues and pull-requests related to new analyzer pr-bugfix Pull request with bugfix, not backported by default v24.3-must-backport pr-must-backport-cloud and removed pr-not-for-changelog This PR should not be mentioned in the changelog labels Apr 8, 2024
@novikd novikd enabled auto-merge April 10, 2024 10:16
@novikd novikd added this pull request to the merge queue Apr 10, 2024
Merged via the queue into ClickHouse:master with commit 14566fc Apr 10, 2024
8 of 18 checks passed
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 10, 2024
@ucasfl ucasfl deleted the analyzer-position-arguments branch April 10, 2024 11:50
robot-clickhouse added a commit that referenced this pull request Apr 10, 2024
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Apr 10, 2024
novikd added a commit that referenced this pull request Apr 17, 2024
Backport #62362 to 24.3: Fix analyzer with positional arguments in distributed query
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer Issues and pull-requests related to new analyzer pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud pr-bugfix Pull request with bugfix, not backported by default pr-must-backport-cloud pr-synced-to-cloud The PR is synced to the cloud repo v24.3-must-backport
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new analyzer: Positional argument is out of bounds in distributed queries
4 participants