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

[PECO-1286] Complex types tests #293

Merged
merged 2 commits into from
Nov 29, 2023
Merged

[PECO-1286] Complex types tests #293

merged 2 commits into from
Nov 29, 2023

Conversation

susodapop
Copy link
Contributor

Description

This PR adds e2e tests for the complex type behaviour of this connector. The test demonstrates that when a user selects a field which contains a complex type value that this type is passed through the result as a List, ndarray, or dict() depending on the type of the field in the table.

This can be disabled with a config when building the connection.

As part of an ongoing effort to improve the "hintability" of this connector in a code editor, I've also extracted _use_arrow_native_complex_types from kwargs and added comments for it.

Jesse Whitehouse added 2 commits November 28, 2023 15:30
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
:param _use_arrow_native_complex_types: `bool`, optional
Controls whether a complex type field value is returned as a string or as a native Arrow type. Defaults to True.
When True:
MAP is returned as List[Tuple[str, Any]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is MAP not returned as a Dict[str, any]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same thought but this seems to be controlled by pyarrow. Updating to use a dict() is a potential improvement we could implement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unclear if it would be an improvement. If this is the native type, let's leave it be.

@susodapop susodapop merged commit aaaf047 into main Nov 29, 2023
12 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
Development

Successfully merging this pull request may close these issues.

2 participants