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

[CALCITE-6497] Use helper setup method throughout whole ElasticsearchAdapterTest #3881

Conversation

timgrein
Copy link
Contributor

I'm working on the LIKE operator for Elasticsearch and noticed that the ElasticSearchAdapterTests are not using a helper method setting up a connection to Elasticsearch consistently, but rather duplicate code sometimes. I want to keep the change separate to not clutter the diff in the LIKE operator PR.

@caicancai
Copy link
Member

caicancai commented Jul 24, 2024

I'm working on the LIKE operator for Elasticsearch and noticed that the ElasticSearchAdapterTests are not using a helper method setting up a connection to Elasticsearch consistently, but rather duplicate code sometimes. I want to keep the change separate to not clutter the diff in the LIKE operator PR.

I think this change can create a jira case to illustrate it.

I see no need to create jira case mainly just fixing some typos and adding some simple tests

@asolimando
Copy link
Member

asolimando commented Jul 24, 2024

I'm working on the LIKE operator for Elasticsearch and noticed that the ElasticSearchAdapterTests are not using a helper method setting up a connection to Elasticsearch consistently, but rather duplicate code sometimes. I want to keep the change separate to not clutter the diff in the LIKE operator PR.

I think this change can create a jira case to illustrate it.

I see no need to create jira case mainly just fixing some typos and adding some simple tests

Agreed, please create a Jira ticket @timgrein, direct PRs are for really minor fixes like typos etc., this one it's worth a ticket IMO

EDIT: alternatively, it can be a separate commit in the other PR at review time, when squashed you can add it in the extended comments (I don't think it will make the PR unreadable, it's a pretty mechanical change in a single class)

@timgrein
Copy link
Contributor Author

Agreed, please create a Jira ticket @timgrein, direct PRs are for really minor fixes like typos etc., this one it's worth a ticket IMO

Gotcha, then I'll create a ticket for this PR and wire it together accordingly!

@timgrein timgrein changed the title Use helper method setting up connection throughout whole ElasticSearchAdapterTest [CALCITE-6497] Use helper setup method throughout whole ElasticsearchAdapterTest Jul 24, 2024
@timgrein timgrein force-pushed the es-adapter-test-use-calcite-assert-helper-method branch from fd4a075 to 04a1cfe Compare July 24, 2024 13:03
@timgrein
Copy link
Contributor Author

Created the ticket https://issues.apache.org/jira/browse/CALCITE-6497 and adjusted the commit message (force-push) to follow the official pattern.

Copy link
Member

@asolimando asolimando left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarcloud bot commented Jul 24, 2024

@asolimando asolimando merged commit 15a5360 into apache:main Jul 24, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants