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

Compare .Kind() instead of direct equality checks on a dyn.Value #1520

Merged
merged 8 commits into from
Jun 27, 2024

Conversation

shreyas-goenka
Copy link
Contributor

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

Changes

This PR makes two changes:

  1. In Track multiple locations associated with a dyn.Value #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 Retain location metadata for values in convert.FromTyped #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 changed the title Use kind instead of direct equality checks to compare with nil and invalid values Compare .Kind() instead of direct equality checks on a dyn.Value Jun 25, 2024
@shreyas-goenka shreyas-goenka marked this pull request as ready for review June 25, 2024 15:36
github-merge-queue bot pushed a commit that referenced this pull request Jun 26, 2024
…1528)

## Changes

This reverts commit dac5f09 (#1523).

Retaining the location for nil values means equality checks no longer
pass.

We need #1520 to be merged first.

## Tests

Integration test `TestAccPythonWheelTaskDeployAndRunWithWrapper`.
bundle/config/mutator/run_as.go Show resolved Hide resolved
@shreyas-goenka shreyas-goenka added this pull request to the merge queue Jun 27, 2024
Merged via the queue into main with commit 4d8eba0 Jun 27, 2024
5 checks passed
@shreyas-goenka shreyas-goenka deleted the remove-direct-dyn-comparision branch June 27, 2024 13:36
pietern added a commit that referenced this pull request Jul 3, 2024
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)).
@pietern pietern mentioned this pull request Jul 3, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 3, 2024
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)).
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.

None yet

2 participants