You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "nealrichardson (via GitHub)" <gi...@apache.org> on 2023/03/15 17:47:08 UTC
[GitHub] [arrow] nealrichardson opened a new pull request, #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
nealrichardson opened a new pull request, #34576:
URL: https://github.com/apache/arrow/pull/34576
### Rationale for this change
Fixes #34519. #33770 introduced the bug; I had [asked](https://github.com/apache/arrow/pull/33770/files#r1081612013) in the review why the C++ function wasn't using `FieldsInExpression`. I swapped that in, and the test I added to reproduce the bug now passes.
### What changes are included in this PR?
Fix for the C++ function, test in R.
### Are these changes tested?
Yes
### Are there any user-facing changes?
The behavior observed in the report no longer happens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1470746859
Ok, I put in some debugging and ran your test case. The way R is building scan options the value of `projection` is not "the output of the scan" but "the output of the scan + project". So, in other words, given `SELECT x, y, x+y` the built plan would be...
```
Scan {
projection=make_struct(field_ref(x), field_ref(y), add(field_ref(y), field_ref(x))
}
Project {
exprs=[
field_ref(0),
field_ref(1),
add(field_ref(1), field_ref(0))
]
}
```
Under the old model we would skip `add(...)` in `ScanOptions::projection` because it was not a field ref. This would have been fine for the above query. However, given the query `SELECT cast(x, string)` then R would give us:
```
Scan {
projection=make_struct(cast(field_ref(x), string))
}
Project {
exprs=[
cast(field_ref(0), string)
]
}
```
However, we would have not realized you wanted to read in any fields at all because we skip `cast(...)`. What I had expected (back when I updated the projection handling code) was something like...
```
Scan {
projection=make_struct(field_ref(x))
}
Project {
exprs=[
cast(field_ref(0), string)
]
}
```
However, that puts the burden of calculating the materialized fields squarely on R's shoulders.
Also, this is more or less the direction the scan node itself is going. In ScanV2 the "projection" will just be a list of "columns to load".
Long term, this seems inevitable. For example, I think this approach may fail with a query like `SELECT left.a from left INNER JOIN right on left.id = right.id WHERE left.b > right.b`. The fact that you have to read `left.b` is not immediately evident until after the join. So I don't think R is going to give us `right.b` in the `projection` for the scan options.
The solution (I think) is a proper "pushdown projection" pass. First, R creates a plan with a scan node that is configured to load everything. Then, in a push down projection pass, the actual scan projection is calculated. Ideally this would be in C++ but it kind of breaks the old philosophy of "zero-optimization"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1470485925
* Closes: #34519
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1478734503
['Python', 'R'] benchmarks have high level of regressions.
[test-mac-arm](https://conbench.ursa.dev/compare/runs/b45cab5cf7104c72a35066ad349ed5d2...f7d6046a95474f92a1f5be0f120bce72/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace merged pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace merged PR #34576:
URL: https://github.com/apache/arrow/pull/34576
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1470912007
I'm fine with this change so don't let this discussion block merging.
> It would, the join code in R ensures that the join keys are present at the join step, then the select of left.a happens after
Right. But I'm more worried about `left.b`. Maybe the case I came up with isn't good enough but I know there are some queries where you will inevitably end up with:
```
Scan -> Project -> Join -> Project
```
What expression is assigned to the scan in that case? Is it the expression from the first projection (would be wrong), the expression from the second projection (would be wrong), or are you actually doing some kind of pushdown logic in R? Or maybe you have some way of preventing this case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1474430019
> Join can only specify join keys based on the columns in Project[0]: if b isn't in Project[0], you can't reference it in Join.
This was the context I was missing (this is different than SQL for example). Given this context I believe your fix will work correctly (I'm going to merge it)
> Further digression: the ScanNode ToString method doesn't print anything useful, so we can't verify what filter and column predicates it has. I thought I filed an issue about that at some point but I can't find it now.
I've added https://github.com/apache/arrow/issues/34625 to make sure we handle this in the new scan node.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1472230841
Further digression: the ScanNode ToString method doesn't print anything useful, so we can't verify what filter and column predicates it has. I thought I filed an issue about that at some point but I can't find it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1472024512
Maybe I misunderstand your concerns, but here's how the R query building works. `select/mutate/filter` build and modify a projection expression and filter expression. At scan time (`collect`), those get pushed down into the ScanNode when querying on a dataset. Aggregations and joins are different, in that they essentially wrap the preceding parts of the query in a black box, and nothing that happens after them modifies any previous steps.
So in the case of
```
Scan -> Project[0] -> Join -> Project[1]
```
the projection of `Project[0]` is pushed into `Scan`. `Join` can only specify join keys based on the columns in `Project[0]`: if `b` isn't in `Project[0]`, you can't reference it in `Join`. We don't (currently) inspect `Project[1]` to see if further columns could be pruned from steps prior to `Join` and pushed down, i.e. `Project[1]` doesn't alter `Project[0]` and thus not `Scan`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1478733549
Benchmark runs are scheduled for baseline = 1f8a335d3bcf728c6377feb352c9001fe04f3fbd and contender = f2c392871d0ec5469d8d85a3fa7a1cea2ddc5d81. f2c392871d0ec5469d8d85a3fa7a1cea2ddc5d81 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/991dd670a0134db683d8809a92e02df7...e64c76a8e98440cba2705a88f60ae049/)
[Finished :arrow_down:0.64% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/b45cab5cf7104c72a35066ad349ed5d2...f7d6046a95474f92a1f5be0f120bce72/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/5d2fc3133cdf437794ca2d3fa9af17aa...5108c2b7786042b28a21954679fe3f11/)
[Finished :arrow_down:0.13% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/4c00c3a90b46464b8ed963ee2afebf23...0a7e8f055c994563bc9543955265d49c/)
Buildkite builds:
[Finished] [`f2c39287` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2549)
[Finished] [`f2c39287` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2579)
[Finished] [`f2c39287` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2547)
[Finished] [`f2c39287` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2570)
[Finished] [`1f8a335d` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2548)
[Finished] [`1f8a335d` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2578)
[Finished] [`1f8a335d` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2546)
[Finished] [`1f8a335d` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2569)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1470486003
:warning: GitHub issue #34519 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #34576: GH-34519: [C++][R] Fix dataset scans that project the same name as a field
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on PR #34576:
URL: https://github.com/apache/arrow/pull/34576#issuecomment-1470763485
> So I don't think R is going to give us left.b in the projection for the scan options.
It would, the join code in R ensures that the join keys are present at the join step, then the select of left.a happens after. Fairly confident we have tests covering this, but easy enough to verify.
If it helps our confidence in this change, this is essentially restoring the code we had in the R C++ bindings before the most recent change, which instead of copying the code from the R package, it adapted a function that already existed in scanner.cc (and apparently wasn't always right).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org