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

fix(stdlib): vectorized map properly handles shadowed columns #4819

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

jsternberg
Copy link
Contributor

@jsternberg jsternberg commented Jun 1, 2022

The vectorized version of map did not properly handle shadowed columns
the way the row based one worked. This copies over some of that code so
that it follows the correct logic. It also copies over the same logic
that is used to determine the column type when type inference does not
properly tell us the type.

This also fixes a bug in building objects that caused a bug in the
compiler. When a with is evaluated, the attributes from the original
record are copied over first and then overwritten values are assigned.
This overwriting did not properly release the values that were being
overwritten resulting in a memory leak.

Fixes #4812.

Done checklist

  • docs/SPEC.md updated
  • Test cases written

@jsternberg jsternberg requested a review from a team as a code owner June 1, 2022 21:30
@jsternberg jsternberg requested review from skartikey and Marwes and removed request for a team and skartikey June 1, 2022 21:30
Copy link
Contributor

@Marwes Marwes left a comment

Choose a reason for hiding this comment

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

I fixed the test I mentioned on slack so that it reproduces the error so if you rebase on #4817 and add this test we have a test to cover that. You seem to have fixed another issue as well though so if you can add a test for that also this looks good 👍

testcase drop_fn_duplicate_column_issue_4812 {
    got = array.from(rows: [{ x: 1, y: "a" }])
        |> map(fn: (r) => ({ r with x: r.x }))
        |> drop(columns: ["y"])

    want = array.from(rows: [{ x: 1 }])

    testing.diff(got, want)
}

@jsternberg jsternberg force-pushed the fix/vectorized-map-coltypes branch from 75a2dea to 14f44eb Compare June 2, 2022 14:20
@jsternberg jsternberg requested a review from a team as a code owner June 2, 2022 14:20
@jsternberg jsternberg requested review from noramullen1 and removed request for a team June 2, 2022 14:20
@jsternberg
Copy link
Contributor Author

Added the test case and also modified some of the code because the new code failed a test.

@jsternberg jsternberg requested a review from Marwes June 2, 2022 14:21
@sanderson sanderson requested review from sanderson and removed request for noramullen1 June 2, 2022 14:21
@jsternberg jsternberg force-pushed the fix/vectorized-map-coltypes branch from 14f44eb to 9f8ab3a Compare June 2, 2022 14:30
The vectorized version of map did not properly handle shadowed columns
the way the row based one worked. This copies over some of that code so
that it follows the correct logic. It also copies over the same logic
that is used to determine the column type when type inference does not
properly tell us the type.

This also fixes a bug in building objects that caused a bug in the
compiler. When a `with` is evaluated, the attributes from the original
record are copied over first and then overwritten values are assigned.
This overwriting did not properly release the values that were being
overwritten resulting in a memory leak.
@jsternberg jsternberg force-pushed the fix/vectorized-map-coltypes branch from 9f8ab3a to dfba1b5 Compare June 2, 2022 14:57
@jsternberg jsternberg merged commit 167cfbf into master Jun 2, 2022
@jsternberg jsternberg deleted the fix/vectorized-map-coltypes branch June 2, 2022 15:16
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.

Vectorized map function should use results for the type instead of type inference
3 participants