PythonMutator: support omitempty in PyDABs #1513

kanterov · 2024-06-20T19:22:37Z

Changes

PyDABs output can omit empty sequences/mappings because we don't track them as optional. There is no semantic difference between empty and missing, which makes omitting correct. CLI detects that we falsely modify input resources by deleting all empty collections.

To handle that, we extend dyn.Override to allow visitors to ignore certain deletes. If we see that an empty sequence or mapping is deleted, we revert such delete.

Tests

Unit tests

codecov-commenter · 2024-06-24T13:19:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.96%. Comparing base (e22dd8a) to head (b1d8b59).
Report is 161 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1513      +/-   ##
==========================================
+ Coverage   52.25%   53.96%   +1.70%     
==========================================
  Files         317      353      +36     
  Lines       18004    20544    +2540     
==========================================
+ Hits         9408    11086    +1678     
- Misses       7903     8653     +750     
- Partials      693      805     +112

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shreyas-goenka · 2024-06-30T23:19:59Z

bundle/config/mutator/python/python_mutator.go

@@ -311,6 +319,21 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor {
 	}
 }

+func isOmitemptyDelete(left dyn.Value) bool {
+	// PyDABs output can omit empty sequences/mappings, because we don't track them as optional,
+	// there is no semantic difference between empty and missing, so we keep them as they were before.


If I understand correctly with this PR you are trying to fix the case where a value in the original bundle config is an empty (but not nil) sequence or mapping, but pydabs skips emitting the key (with an empty value) in its serialized output. This causes an error (unexpected change at %q (delete)) which you want to avoid.

Is my understanding of the problem you are trying to solve correct?

Correct. We don't distinguish between empty and unset lists, so when the list is empty, we omit it in the output.

pietern

The change LGTM, but can you elaborate on why we don't treat these as different? If the JSON representation is mapped to a Python data class, I imagine there in fact being a difference between a None field and an empty list.

libs/dyn/merge/override.go

kanterov · 2024-07-01T09:06:41Z

@pietern having Optional[list[T]] was not helpful because any Python code (including user-specified) had to handle the possibility of missing values while there is no semantic difference in it. It also made many type signatures unnecessarily complex. So we only left it in "create" method signatures to avoid problems with mutating arguments.

In cases where it matters, we can still use Optional[list[T]] in data classes, and it will keep missing values as-is.

We can fix it in a different way by tracking which lists were not modified or ever specified explicitly.

pietern · 2024-07-02T08:04:44Z

bundle/config/mutator/python/python_mutator.go

@@ -311,6 +319,21 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor {
 	}
 }

+func isOmitemptyDelete(left dyn.Value) bool {


omitempty is specific to the JSON serializer. Here we check if the incoming value is an empty collection.

Maybe isEmptyCollection?

This was intentional because we want to reserve this method to specific behavior with omit empty and add a comment explaining that. For instance, we intentionally don't handle objects where all fields are empty.

libs/dyn/merge/override.go

pietern · 2024-07-02T08:06:18Z

libs/dyn/merge/override.go

+// the final value of the node. 'VisitDelete' should return dyn.NilValue to
+// do the delete, or can return 'left' to undo it.
+//
+// TODO 'VisitDelete' and 'VisitInsert' should support dyn.NilValue as well


What would the semantics be?

I think you mean VisitUpdate here, not VisitDelete.

pietern · 2024-07-02T08:07:02Z

libs/dyn/merge/override.go


 			if err != nil {
 				return dyn.NewMapping(), err
 			}
+
+			// if 'delete' was undone, add it back
+			if deleteOut != dyn.NilValue {


You need to check the Kind(). A nil value may have a location attached to it so direct equality can fail.

pietern · 2024-07-02T08:07:12Z

libs/dyn/merge/override.go


 			if err != nil {
 				return nil, err
 			}
+
+			// if 'delete' was undone, add it back
+			if out != dyn.NilValue {


kanterov · 2024-07-02T11:27:57Z

@pietern, thanks for the suggestions about using errors. It's much cleaner now, and we don't need to document that insert/update doesn't support this behavior because the error is dedicated to the "delete" method.

libs/dyn/merge/override.go

pietern · 2024-07-03T07:02:11Z

bundle/config/mutator/python/python_mutator.go

+	if left.Kind() == dyn.KindNil {
+		return true
+	}
+


A switch/case would be cleaner here, IMO.

Co-authored-by: Pieter Noordhuis <[email protected]>

Bundles: As of this release you can interact with bundles when running the CLI on DBR (e.g. via the Web Terminal). * Fix non-default project names not working in dbt-sql template ([#1500](#1500)). * Improve `bundle validate` output ([#1532](#1532)). * Fixed resolving variable references inside slice variable ([#1550](#1550)). * Fixed bundle not loading when empty variable is defined ([#1552](#1552)). * Use `vfs.Path` for filesystem interaction ([#1554](#1554)). * Replace `vfs.Path` with extension-aware filer when running on DBR ([#1556](#1556)). Internal: * merge.Override: Fix handling of dyn.NilValue ([#1530](#1530)). * Compare `.Kind()` instead of direct equality checks on a `dyn.Value` ([#1520](#1520)). * PythonMutator: register product in user agent extra ([#1533](#1533)). * Ignore `dyn.NilValue` when traversing value from `dyn.Map` ([#1547](#1547)). * Add extra tests for the sync block ([#1548](#1548)). * PythonMutator: add diagnostics ([#1531](#1531)). * PythonMutator: support omitempty in PyDABs ([#1513](#1513)). * PythonMutator: allow insert 'resources' and 'resources.jobs' ([#1555](#1555)).

kanterov force-pushed the python-mutator-ommitempty branch 4 times, most recently from a8c5352 to b695d5d Compare June 24, 2024 13:15

kanterov force-pushed the python-mutator-ommitempty branch from b695d5d to fb6c851 Compare June 24, 2024 13:19

PythonMutator: support omitempty in PyDABs

b1d8b59

kanterov force-pushed the python-mutator-ommitempty branch from fb6c851 to b1d8b59 Compare June 24, 2024 13:22

kanterov requested a review from pietern June 24, 2024 13:23

shreyas-goenka reviewed Jun 30, 2024

View reviewed changes

kanterov requested a review from shreyas-goenka July 1, 2024 06:20

pietern reviewed Jul 1, 2024

View reviewed changes

libs/dyn/merge/override.go Outdated Show resolved Hide resolved

kanterov requested a review from pietern July 1, 2024 11:43

pietern reviewed Jul 2, 2024

View reviewed changes

kanterov added 7 commits July 2, 2024 11:39

Use err to undo deletes

7a3e961

Fix tests

370b286

fmt

838c116

Update comment

214577b

fmt

f067029

more fmt

d58f374

Remove redundant assignment

0ffa1b6

kanterov requested a review from pietern July 2, 2024 11:28

kanterov and others added 2 commits July 2, 2024 17:24

Merge branch 'main' into python-mutator-ommitempty

f3b352e

Address more comments

dd4583e

pietern approved these changes Jul 3, 2024

View reviewed changes

kanterov and others added 2 commits July 3, 2024 09:11

Update libs/dyn/merge/override.go

03994b6

Co-authored-by: Pieter Noordhuis <[email protected]>

Use switch

e575c97

pietern enabled auto-merge July 3, 2024 07:17

pietern added this pull request to the merge queue Jul 3, 2024

Merged via the queue into databricks:main with commit b9e3c98 Jul 3, 2024
5 checks passed

pietern mentioned this pull request Jul 3, 2024

Release v0.223.0 #1557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PythonMutator: support omitempty in PyDABs #1513

PythonMutator: support omitempty in PyDABs #1513

kanterov commented Jun 20, 2024 •

edited

Loading

codecov-commenter commented Jun 24, 2024 •

edited

Loading

shreyas-goenka Jun 30, 2024

kanterov Jul 1, 2024

pietern left a comment

kanterov commented Jul 1, 2024

pietern Jul 2, 2024

kanterov Jul 2, 2024

pietern Jul 2, 2024

pietern Jul 2, 2024

pietern Jul 2, 2024

kanterov commented Jul 2, 2024

pietern Jul 3, 2024

PythonMutator: support omitempty in PyDABs #1513

PythonMutator: support omitempty in PyDABs #1513

Conversation

kanterov commented Jun 20, 2024 • edited Loading

Changes

Tests

codecov-commenter commented Jun 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

kanterov commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanterov commented Jul 2, 2024

Choose a reason for hiding this comment

kanterov commented Jun 20, 2024 •

edited

Loading

codecov-commenter commented Jun 24, 2024 •

edited

Loading