Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 tests #357
base: master
Are you sure you want to change the base?
Fix tests #357
Changes from 4 commits
b791e35
a7405bc
4732612
84831a9
9eb1f45
1914607
9ea84b8
17e763e
456aa91
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Without:
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.
But with the experimental JSON support enabled, another exceptions occurs:
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.
@9en9i , could you play with this branch locally and, if you can find a solution better than just skipping the test here, this PR, when merged, allow to undraft the PR #356 .
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.
Okay, I ran the tests with three different ClickHouse versions before all your commits:
• 22.5.1.2079 (GitHub Actions version): The test was skipped with the message “Minimum revision required: 22.6.1”.
• 23.8.4.69 (also from GitHub Actions): The test passed.
• 24.10 (from your logs): The test failed.
It looks like ClickHouse changed the flag name from allow_experimental_object_type to allow_experimental_json_type in one of the newer versions.
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.
Also, judging by the logs, the type was previously called Object('json'), but now it’s just JSON.
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.
@9en9i , are you able to approve this PR?
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'm not sure it's needed as it is now. Ideally, a pull request should focus on one thing. I don't see any specific goal here. I suggest we withdraw it for now and focus and concentrate on ending support for python 3.8. But we need the opinion and approval of the maintainer anyway
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 specific goal here is to make tests pass. This PR is kind of flaky, I admit it, because JSON failure is not gone yet. Also, in the #356 I haven't seen any reports about the pipeline that either failed or succeeded.
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.
Then maybe we can start by trying to finish with #356. Can you change his status?
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.
Ok, so it be ready for review.