Skip to content

[PECOBLR-330] Support for complex params #559

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

jprakash-db
Copy link
Contributor

@jprakash-db jprakash-db commented May 19, 2025

Description

This PR introduces comprehensive support for complex parameter types (ARRAY and MAP) in the Databricks SQL Python client.

Key Changes

  • Added new parameter classes: ArrayParameter and MapParameter
  • These classes recursively wrap Python lists and dictionaries and serialize them into the appropriate Thrift structures.
  • The type inference logic dbsql_parameter_from_primitive is updated to recognize and construct these parameter types, including for nested complex values.
  • Added the TSparkParamValueArgs which is necessary to be passed as arguements for complex types

Testing

e2e

  • Added tests to insert and compare the results for nested structures ARRAY<ARRAY<STRING>>, ARRAY<MAP<STRING,INTEGER>>, MAP<STRING,ARRAY<STRING>>

unit

  • Added tests to verify the convertion of the params into the respective server contract format

IGNORE THE FAILING e2e Tests

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Working e2e tests prototype
Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@jprakash-db jprakash-db self-assigned this Jun 6, 2025
@jprakash-db jprakash-db marked this pull request as ready for review June 6, 2025 06:48
@jprakash-db jprakash-db changed the title Support for complex params [PECOBLR-330] Support for complex params Jun 6, 2025
@@ -429,7 +429,7 @@ def user_friendly_error_message(self, no_retry_reason, attempt, elapsed):
# Taken from PyHive
class ParamEscaper:
_DATE_FORMAT = "%Y-%m-%d"
_TIME_FORMAT = "%H:%M:%S.%f"
_TIME_FORMAT = "%H:%M:%S.%f %z"
Copy link
Contributor

Choose a reason for hiding this comment

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

Timezone will not be there for TIMESTAMP_NTZ param.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, would like to know how we've accounted/tested for NTZ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is not there it will be an empty space, there is already a test suite that inserts NTZ and none NTZ and reads back to compare whether it is equal or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vikrantpuppala @shivam2680 There are already existing tests that insert NTZ and non NTZ values and reads back from table to ensure everything is working as expected -

def test_dbsqlparameter_single(

@@ -429,7 +429,7 @@ def user_friendly_error_message(self, no_retry_reason, attempt, elapsed):
# Taken from PyHive
class ParamEscaper:
_DATE_FORMAT = "%Y-%m-%d"
_TIME_FORMAT = "%H:%M:%S.%f"
_TIME_FORMAT = "%H:%M:%S.%f %z"
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, would like to know how we've accounted/tested for NTZ

Copy link

@gopalldb gopalldb left a comment

Choose a reason for hiding this comment

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

Is this not an opt-in feature, can't user disable this in favor of existing behavior?

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@jprakash-db
Copy link
Contributor Author

@gopalldb We are just adding support for more types, don't think there is anything to opt in. If the users don't use complex types they won't need this

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@jprakash-db jprakash-db deployed to azure-prod June 13, 2025 12:11 — with GitHub Actions Active
Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

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.

4 participants