Marshalling: allow JSON-like fields #241
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nfiguration files. This is a convenience value, and represents the maximum bounds for a value that can be natively serialised/deserialised using the YAML format that we use.
It was being handled in two-places, which wasn't necessary.
… should be refactored.
JSON represents the bounds of what can be serialised.
JSONValue can be used as a drop-in replacement.
asnare
force-pushed
the
support-marshalling-json
branch
from
c416539 to
8ddfd96
Compare
asnare
commented
May 23, 2025
…eserialized. Previously the field would be reinitialized with its default value, if possible.
Reduces complexity of the main unmarshalling function.
gueniai
approved these changes
May 27, 2025
sundarshankar89
added a commit
that referenced
this pull request
May 27, 2025
* Marshalling: allow JSON-like fields ([#241](#241)). The library has undergone significant changes to improve its marshalling functionality, code readability, and maintainability. A new `JsonValue` type alias has been introduced to represent the maximum bounds of values that can be saved for an installation, and support for `Any` and `object` as type annotations on data classes has been removed. The library now issues a `DeprecationWarning` when saving raw `list` and `dict` fields, and raises a specific error during loading, instructing users to use `list[T]` or `dict[T]` instead. Various methods, including `_marshal_generic_list`, `_marshal_raw_list`, `_marshal_generic_dict`, and `_marshal_raw_dict`, have been updated to handle the serialization of lists and dictionaries, while the `_unmarshal` method now handles the deserialization of unions, lists, and dictionaries. Additionally, the library has been updated to provide more informative error messages, and several tests have been added to cover various scenarios, including generic dict and list JSON values, bool in union, and raw list and dict deprecation. The `Installation` class, `MockInstallation` class, and `Paths` class have also been updated with new methods, type hints, and custom initialization to improve code flexibility and maintainability.
Merged
sundarshankar89
added a commit
that referenced
this pull request
May 27, 2025
* Marshalling: allow JSON-like fields ([#241](#241)). The library has undergone significant changes to improve its marshalling functionality, code readability, and maintainability. A new `JsonValue` type alias has been introduced to represent the maximum bounds of values that can be saved for an installation, and support for `Any` and `object` as type annotations on data classes has been removed. The library now issues a `DeprecationWarning` when saving raw `list` and `dict` fields, and raises a specific error during loading, instructing users to use `list[T]` or `dict[T]` instead. Various methods, including `_marshal_generic_list`, `_marshal_raw_list`, `_marshal_generic_dict`, and `_marshal_raw_dict`, have been updated to handle the serialization of lists and dictionaries, while the `_unmarshal` method now handles the deserialization of unions, lists, and dictionaries. Additionally, the library has been updated to provide more informative error messages, and several tests have been added to cover various scenarios, including generic dict and list JSON values, bool in union, and raw list and dict deprecation. The `Installation` class, `MockInstallation` class, and `Paths` class have also been updated with new methods, type hints, and custom initialization to improve code flexibility and maintainability.
github-merge-queue bot
pushed a commit
to databrickslabs/lakebridge
that referenced
this pull request
May 28, 2025
This PR continues from #1488, introducing the following: - The `--debug` handling introduced in #1488 is removed. This was a workaround for a problem solved upstream in #238. - The logging has been adjusted to reflect the updates we made in #1576. - A test from #1568 is fixed; something on this branch leads the version information to include a timestamp, which needs to be ignored by that test. - Some existing (installation) tests were incorrectly mocking the install-state: the values should always be strings, but the tests were placing integers in the fixture. Some upstream changes on #189 change the behaviour in this area. ### Implementation details ~This pull-request depends on databrickslabs/blueprint#241, and the project dependencies have been updated to depend on them. This is a temporary change to ensure that CI can work. Before merging a release of blueprint is required, and the released version needs to be used instead.~ [Done] ### Caveats/things to watch out for when reviewing: Please check the changes to the `transpile` command. ### Linked issues Follows #1488. ### Functionality - modified existing command: `databricks labs remorph transpile` (from #1488) ### Tests - existing unit tests - added integration tests --------- Co-authored-by: Eric Vergnaud <eric.vergnaud@databricks.com> Co-authored-by: sundar.shankar <sundar.shankar@databricks.com>
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR continues from #189 by updating it with the following changes:
JsonValuetype alias that represents the maximum bounds of the values that can be saved for an installation.Anyandobjectas type annotations on data classes that we marshal. The existing tests for these have been updated to useJsonValueinstead.listanddictfields is deprecated. Saving these used to work, but loading would fail. I've chosen to deal with this by:list[T]ordict[T]instead..save()for what it can actually handle.Anybut really only accepted a subset.This is sufficient to allow the use-case that we need in databrickslabs/lakebridge#1488, and is preferred to the approach in #189 because it forces developers to fix their code:
JSONValueas a field will let the type-checker tell them if a value is assigned that does not conform. (The fix is for the client code to validate the values.)One of the bugs fixed in #189 relates to automatic conversion between JSON values: for example, from
123to"123"or vice-versa. Some tests were (incorrectly) relying on this, as were some tests in a downstream project (remorph). There might be other places in production code where the previous behaviour is relied upon. As such, the next release that includes this code (or that from #189) should bump the minor version and not just be a patch release.