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

Use demand in vector.read #4850

Merged
merged 27 commits into from
Nov 7, 2023
Merged

Use demand in vector.read #4850

merged 27 commits into from
Nov 7, 2023

Conversation

jamii
Copy link
Contributor

@jamii jamii commented Nov 3, 2023

Hooking up demand to vector.Read.

  • Use demand in vector.Read to avoid reading unneeded vectors. Also changes the type of the output value, removing any unused fields so that we can avoid having to materialize a bunch of nulls.
  • Add vngio.ReaderOpts, where opts.Demand defaults to demand.All{}.
  • Split the demand representation into a separate package to avoid import loops.
  • Pull the fuzzer code from Fuzz vng roundtrip #4833 into a fuzz package. (I've manually copied across the requested changes from that pr).
  • Add a fuzzer that runs random queries through both zng and vng paths and compares the results. (Found value.Under loops forever if called with null union #4854). This uses a gross hack to get around the fact that readers are created before queries are analyzed.
  • Change the constants vector to hold []byte instead of zed.Value, since all we ever do is copy the bytes out.

This doesn't yet actually do anything for zq or zed query. I can work on the latter, and @nwt is thinking about how to refactor to make zq possible.

@jamii jamii force-pushed the jamii-vector-demand branch 4 times, most recently from d7aa200 to 451f865 Compare November 5, 2023 18:33
@jamii jamii changed the title WIP vector demand Use demand in vector.read Nov 5, 2023
@jamii jamii marked this pull request as ready for review November 5, 2023 18:50
@jamii jamii requested a review from a team November 5, 2023 18:51
@jamii
Copy link
Contributor Author

jamii commented Nov 5, 2023

@brimdata/zed This is ready for review, but before merging I'll need to rebase it on #4833 to make the vng tests use the new fuzz package.

@jamii jamii force-pushed the jamii-vector-demand branch from 6c2f836 to e1f249d Compare November 6, 2023 23:26
@jamii jamii force-pushed the jamii-vector-demand branch from 72d0e38 to 93f3b0d Compare November 6, 2023 23:40
@jamii
Copy link
Contributor Author

jamii commented Nov 6, 2023

@nwt I finished rebasing this on the other two PRs.

zio/vngio/reader.go Outdated Show resolved Hide resolved
compiler/optimizer/demand/demand.go Outdated Show resolved Hide resolved
compiler/optimizer/demand/demand.go Outdated Show resolved Hide resolved
compiler/optimizer/demand/demand.go Outdated Show resolved Hide resolved
zio/vngio/reader.go Outdated Show resolved Hide resolved
vector/materializer.go Outdated Show resolved Hide resolved
vector/read.go Outdated Show resolved Hide resolved
vector/read.go Outdated Show resolved Hide resolved
fuzz/fuzz.go Outdated Show resolved Hide resolved
jamii and others added 2 commits November 7, 2023 08:28
Copy link
Member

@nwt nwt left a comment

Choose a reason for hiding this comment

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

I'd like to have the NewReader(zctx *zed.Context, r io.Reader, d demand.Demand) change but then :shipit:.

zio/vngio/reader.go Outdated Show resolved Hide resolved
return a.Values(), nil
}

func WriteZNG(t *testing.T, valuesIn []zed.Value, buf *bytes.Buffer) {
Copy link
Member

Choose a reason for hiding this comment

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

Nits:

  1. Could use io.Writer here to make this a little more flexible.
  2. We usually put the destination parameter first (as with zio.Copy).
  3. We usually use val as the name for zed.Value slice.
Suggested change
func WriteZNG(t *testing.T, valuesIn []zed.Value, buf *bytes.Buffer) {
func WriteZNG(t *testing.T, w io.Writer, vals []zed.Value) {

@jamii jamii force-pushed the jamii-vector-demand branch from 9566145 to 2c177b1 Compare November 7, 2023 21:22
@jamii jamii merged commit 3753828 into main Nov 7, 2023
3 checks passed
@jamii jamii deleted the jamii-vector-demand branch November 7, 2023 21:38
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.

2 participants