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

Track multiple locations associated with a dyn.Value #1510

Merged
merged 24 commits into from
Jul 16, 2024

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Jun 20, 2024

Changes

This PR changes the location metadata associated with a dyn.Value to a slice of locations. This will allow us to keep track of location metadata across merges and overrides.

The convention is to treat the first location in the slice as the primary location. Also, the semantics are the same as before if there's only one location associated with a value, that is:

  1. For complex values (maps, sequences) the location of the v1 is primary in Merge(v1, v2)
  2. For primitive values the location of v2 is primary in Merge(v1, v2)

Tests

Modifying existing merge unit tests. Other existing unit tests and integration tests pass.

@shreyas-goenka
Copy link
Contributor Author

shreyas-goenka commented Jun 20, 2024

patch file used:

@@
var x expression
@@
-dyn.NewValue(x, dyn.Location{})
+dyn.V(x)

@@
var x expression
var y identifier
@@
-dyn.NewValue(x, y)
+dyn.NewValue(x, []dyn.Location{y})

@@
var x expression
@@
-dyn.NewValue(x, dyn.Location{...})
+dyn.NewValue(x, []dyn.Location{{...}})

@@
var x expression
var y expression
@@
-dyn.NewValue(x, y.Location())
+dyn.NewValue(x, y.Locations())

@@
var x expression
var y expression
@@
-NewValue(x, y.Location())
+NewValue(x, y.Locations())

@@
@@
-dyn.NilValue.WithLocation(...)
+dyn.NilValue.WithLocations([]dyn.Location{...})

github-merge-queue bot pushed a commit that referenced this pull request Jun 27, 2024
…1520)

## Changes

This PR makes two changes:

1. In #1510 we'll be adding
multiple associated location metadata with a dyn.Value. The Go compiler
does not allow comparing structs if they contain slice values
(presumably due to multiple possible definitions for equality). In
anticipation for adding a `[]dyn.Location` type field to `dyn.Value`
this PR removes all direct comparisons of `dyn.Value` and instead relies
on the kind.

2. Retain location metadata for values in convert.FromTyped. The change
diff is exactly the same as #1523.
It's been combined with this PR because they both depend on each other
to prevent test failures (forming a test failure deadlock).

Go patch used:
```
@@
var x expression
@@
-x == dyn.InvalidValue
+x.Kind() == dyn.KindInvalid

@@
var x expression
@@
-x != dyn.InvalidValue
+x.Kind() != dyn.KindInvalid

@@
var x expression
@@
-x == dyn.NilValue
+x.Kind() == dyn.KindNil

@@
var x expression
@@
-x != dyn.NilValue
+x.Kind() != dyn.KindNil
```
 

## Tests
Unit tests and integration tests pass.
@shreyas-goenka shreyas-goenka marked this pull request as ready for review July 1, 2024 13:22
@shreyas-goenka
Copy link
Contributor Author

Once this is merged, I'll followup with the first use case for this: #1506

libs/dyn/merge/merge.go Outdated Show resolved Hide resolved
libs/dyn/merge/merge_test.go Show resolved Hide resolved
libs/dyn/value.go Outdated Show resolved Hide resolved
libs/dyn/value_test.go Outdated Show resolved Hide resolved
libs/dyn/value.go Outdated Show resolved Hide resolved
libs/dyn/value.go Outdated Show resolved Hide resolved
libs/dyn/value.go Show resolved Hide resolved
libs/dyn/merge/merge.go Outdated Show resolved Hide resolved
@pietern pietern changed the title Track multiple locations associated with a dyn.Value Track multiple locations associated with a dyn.Value Jul 2, 2024
libs/dyn/merge/merge.go Outdated Show resolved Hide resolved
libs/dyn/merge/merge_test.go Outdated Show resolved Hide resolved
libs/dyn/value.go Outdated Show resolved Hide resolved
@shreyas-goenka shreyas-goenka requested a review from pietern July 5, 2024 08:32
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewnester Can you TAL as well?

//
// - Merging primitive values means using the incoming value `b`. The location of the
// incoming value is retained and the location of the existing value `a` is accumulated.
// This is because the incoming value overwrites the existing value.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is clear, thanks.

libs/dyn/merge/override.go Show resolved Hide resolved
@shreyas-goenka shreyas-goenka added this pull request to the merge queue Jul 16, 2024
Merged via the queue into main with commit 8ed9964 Jul 16, 2024
5 checks passed
@shreyas-goenka shreyas-goenka deleted the feature/track-locations branch July 16, 2024 11:36
andrewnester added a commit that referenced this pull request Jul 18, 2024
CLI:
 * [Fix] Do not buffer files in memory when downloading ([#1599](#1599)).

Bundles:
 * Allow artifacts (JARs, wheels) to be uploaded to UC Volumes ([#1591](#1591)).
 * Upgrade TF provider to 1.48.3 ([#1600](#1600)).
 * Fixed job name normalisation for bundle generate ([#1601](#1601)).

Internal:
 * Add UUID to uniquely identify a deployment state ([#1595](#1595)).
 * Track multiple locations associated with a `dyn.Value` ([#1510](#1510)).
 * Attribute Terraform API requests the CLI ([#1598](#1598)).
 * Use local Terraform state only when lineage match ([#1588](#1588)).
 * Implement readahead cache for Workspace API calls ([#1582](#1582)).

Dependency updates:
 * Bump github.com/databricks/databricks-sdk-go from 0.43.0 to 0.43.2 ([#1594](#1594)).
@andrewnester andrewnester mentioned this pull request Jul 18, 2024
andrewnester added a commit that referenced this pull request Jul 18, 2024
CLI:
 * Do not buffer files in memory when downloading ([#1599](#1599)).

Bundles:
 * Allow artifacts (JARs, wheels) to be uploaded to UC Volumes ([#1591](#1591)).
 * Upgrade TF provider to 1.48.3 ([#1600](#1600)).
 * Fixed job name normalisation for bundle generate ([#1601](#1601)).

Internal:
 * Add UUID to uniquely identify a deployment state ([#1595](#1595)).
 * Track multiple locations associated with a `dyn.Value` ([#1510](#1510)).
 * Attribute Terraform API requests the CLI ([#1598](#1598)).
 * Implement readahead cache for Workspace API calls ([#1582](#1582)).
 * Use local Terraform state only when lineage match ([#1588](#1588)).

Dependency updates:
 * Bump github.com/databricks/databricks-sdk-go from 0.43.0 to 0.43.2 ([#1594](#1594)).
@andrewnester andrewnester mentioned this pull request Jul 18, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 18, 2024
CLI:
* Do not buffer files in memory when downloading
([#1599](#1599)).

Bundles:
* Allow artifacts (JARs, wheels) to be uploaded to UC Volumes
([#1591](#1591)).
* Upgrade TF provider to 1.48.3
([#1600](#1600)).
* Fixed job name normalisation for bundle generate
([#1601](#1601)).

Internal:
* Add UUID to uniquely identify a deployment state
([#1595](#1595)).
* Track multiple locations associated with a `dyn.Value`
([#1510](#1510)).
* Attribute Terraform API requests the CLI
([#1598](#1598)).
* Implement readahead cache for Workspace API calls
([#1582](#1582)).
* Use local Terraform state only when lineage match
([#1588](#1588)).
* Add read-only mode for extension aware workspace filer
([#1609](#1609)).


Dependency updates:
* Bump github.com/databricks/databricks-sdk-go from 0.43.0 to 0.43.2
([#1594](#1594)).
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