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

added option to disable flatten nested #63

Merged

Conversation

tmahany419
Copy link
Contributor

SUMMARY

I could not find a way to run multiple queries from the same context using this module. There's no way to create a table with flatten_nested disabled without running multiple queries. This PR adds a field called flatten_nested for clickhouse_client that sets this setting before running the query.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

Flatten Nested

ADDITIONAL INFORMATION

This is clickhouse's document explaining options for nested fields: https://clickhouse.com/docs/en/sql-reference/data-types/nested-data-structures/nested. The behavior with flatten_nested enabled and disabled is explained there. The only way to disable it is by running set flatten_nested=0; before creating a table.

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

plugins/doc_fragments/client_inst_opts.py Outdated Show resolved Hide resolved
plugins/module_utils/clickhouse.py Outdated Show resolved Hide resolved
plugins/modules/clickhouse_client.py Outdated Show resolved Hide resolved
@tmahany419
Copy link
Contributor Author

@Andersson007 Thank you for the feedback! I committed the changes you proposed. I've made unit test and it works against a local clickhouse db. I added it to the file you linked in the branch I am working on. Is there a way to test it before updating the PR?

@Andersson007
Copy link
Contributor

@Andersson007 Thank you for the feedback! I committed the changes you proposed. I've made unit test and it works against a local clickhouse db. I added it to the file you linked in the branch I am working on. Is there a way to test it before updating the PR?

@tmahany419 thanks!
About testing, yes, you can run the tests locally before pushing, please take a look at https://docs.ansible.com/ansible/devel/community/create_pr_quick_start.html , there are corresponding sections there

@Andersson007
Copy link
Contributor

@tmahany419 you still need to make CI green before we can consider merging the PR, try to start with merging my suggesting above, maybe it'll fix some failures

@tmahany419
Copy link
Contributor Author

@Andersson007 I merged the change you mentioned. I've also added a test as requested.

@Andersson007
Copy link
Contributor

@tmahany419 I don't see the tests, maybe you forgot to add a change to your commit?

@tmahany419
Copy link
Contributor Author

@Andersson007 I updated the wrong branch yesterday, it should be there now.

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@tmahany419 there's still unit tests failures but I'll fix them myself.
Thanks a lot for the contribution!
You're welcome in future for more contributions/feedback!

@Andersson007 Andersson007 merged commit d84a69d into ansible-collections:main Jul 3, 2024
17 of 21 checks passed
@aleksvagachev
Copy link
Collaborator

@Andersson007 Maybe it is more correct to implement the transfer of not a specific parameter, but the desired list of parameters?

@Andersson007
Copy link
Contributor

Andersson007 commented Jul 4, 2024

@aleksvagachev good catch, something like the following

- name: Run the query
  community.clickhouse.clickhouse_client:
  query: SELECT ...
  set:
    flatten_nested: 0

?

@aleksvagachev
Copy link
Collaborator

@Andersson007 Yes, something similar. Shall we try to implement this in 1.0.0?

@Andersson007
Copy link
Contributor

@Andersson007 Yes, something similar. Shall we try to implement this in 1.0.0?

@aleksvagachev as this change hasn't been released yet, we can do it right now. Thoughts? Would you like to take it?

@aleksvagachev
Copy link
Collaborator

@Andersson007 I'll try to do it, but not before Monday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants