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

Primitive values dropped when "this" passed into User Op with spread #4650

Closed
philrz opened this issue Jun 13, 2023 · 1 comment · Fixed by #4663
Closed

Primitive values dropped when "this" passed into User Op with spread #4650

philrz opened this issue Jun 13, 2023 · 1 comment · Fixed by #4663
Assignees

Comments

@philrz
Copy link
Contributor

philrz commented Jun 13, 2023

Repro is with Zed commit 2314053.

The main scope is able to process both primitive and complex values as this, e.g.:

$ cat kind-in-main.zed
yield {kind: kind(this),ThisWas: this}

$ echo '{foo: "bar"} "hello"' | zq -z -I kind-in-main.zed -
{kind:"record",ThisWas:{foo:"bar"}}
{kind:"primitive",ThisWas:"hello"}

However, if I wrap that yield inside a User Op and try to pass this from main scope into the Op's this using the ... spread operator, the primitive value gets dropped.

$ cat kind-in-op.zed 
op CallMe(...): (
  yield {kind: kind(this),ThisWas: this}
)
CallMe(this)

$ echo '{foo: "bar"} "hello"' | zq -z -I kind-in-op.zed -
{kind:"record",ThisWas:{foo:"bar"}}
{kind:"record",ThisWas:{}}

Once a user is aware of this limitation, they could work around it by passing the main scope's this into a named parameter, e.g.:

$ cat kind-in-op-nested.zed
op CallMe(nested): (
  yield {kind: kind(nested),ThisWas: nested}
)
CallMe(this)

$ echo '{foo: "bar"} "hello"' | zq -z -I kind-in-op-nested.zed -
{kind:"record",ThisWas:{foo:"bar"}}
{kind:"primitive",ThisWas:"hello"}

Admittedly, the docs do call out how the use of the spread operator means that only records will be expected:

...user-defined operators may use the spread operator ... to indicate that the operator expects a record value whose key/values will be expanded as entries in the operator's this record value.

However, if we start from the design principle stated in #4649, we may want to avoid having this limitation. When discussing as a team, there were two reactions:

  1. If we kept the current behavior, it could be improved by throwing an error when such values are dropped rather than its current silence
  2. We may instead change to an approach that allows this to passed in from the main scope in a way that primitive values don't get dropped
@philrz
Copy link
Contributor Author

philrz commented Jun 22, 2023

Verified in Zed commit e57e90e.

Of the two likely approaches described above, the changes in linked PR #4650 took the latter approach of allowing this to be carried through into the User Op such that primitive values are no longer dropped. Updating the repro steps to align with the revised syntax, we now see:

$ zq -version
Version: v1.8.1-37-ge57e90ee

$ cat kind-in-op-revised.zed 
op CallMe(): (
  yield {kind: kind(this),ThisWas: this}
)
CallMe()

$ echo '{foo: "bar"} "hello"' | zq -z -I kind-in-op-revised.zed -
{kind:"record",ThisWas:{foo:"bar"}}
{kind:"primitive",ThisWas:"hello"}

Thanks @mattnibs!

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 a pull request may close this issue.

2 participants