You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/11/18 10:00:00 UTC
[GitHub] [beam] judahrand opened a new pull request, #24249: `COALESCE` should infer correct non-null return type
judahrand opened a new pull request, #24249:
URL: https://github.com/apache/beam/pull/24249
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
damccorm commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1319965863
Sorry, I wasn't clear. I'm not making a claim about whether or not this is actively an issue; I was just saying that given the conversation around the calcite upgrade I'm surprised that it is still a problem. I would have expected that upgrade to fix the problem given my loose understanding of the issue
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
damccorm commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1335667106
Looks like Andrew is on vacation, @kileys could you weigh in here?
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1319870258
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1580738717
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] judahrand commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
judahrand commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1319964333
This is another important detail for our particular query: according to the ZetaSQL spec[1] a `NULL` array and an empty array are distinct and so our query should work.
[1] https://github.com/google/zetasql/blob/master/docs/data-types.md#nulls-and-the-array-type
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1319823160
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @robertwb for label java.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] judahrand commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
judahrand commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1319954241
> From the conversation in https://issues.apache.org/jira/browse/BEAM-7610 and related PRs, this should have been fixed by #14146
>
> @apilloud could you take a look at this one since you have more context here?
It's an issue we're definitely having in Dataflow SQL. A query which looks roughly like this:
```
SELECT
window_start,
window_end,
COALESCE(ARRAY_AGG(foo), []) AS data
FROM HOP(
(SELECT * FROM pubsub.topic.`project_id`.`topic`),
DESCRIPTOR(event_timestamp),
"INTERVAL 5 MINUTE",
"INTERVAL 15 MINUTE"
) as data
GROUP BY window_start, window_end
```
results in an error downstream which says that Data Catalog does not support `NULLABLE ARRAYS`. This means that the output schema of the query must not be correctly handling the `COALESCE`.
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] closed pull request #24249: `COALESCE` should infer correct non-null return type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #24249: `COALESCE` should infer correct non-null return type
URL: https://github.com/apache/beam/pull/24249
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] judahrand commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
judahrand commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1319869295
R: @damccorm
I'm struggling to get the tests to run locally on an M1 MacBook so was hoping to be able to use the CI to demonstrate the issue. But it looks like the extensions unit tests don't run by default.
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] judahrand commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
judahrand commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1319957726
In fact, does the basic test not prove that this is still broken?
https://github.com/apache/beam/blob/cf56af2799f61dd4fc02923e7f23f421723765c6/sdks/java/extensions/sql/zetasql/src/test/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSqlDialectSpecTest.java#L458-L476
Given that the final value of the `COALESCE` in this test is a `STRING` the return type should be `STRING NOT NULL` but the test asserts that it is `NULLABLE`.
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1427024023
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] judahrand commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by "judahrand (via GitHub)" <gi...@apache.org>.
judahrand commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1427026199
> This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [dev@beam.apache.org](mailto:dev@beam.apache.org) list. Thank you for your contributions.
I think this is still an issue.
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] apilloud commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
apilloud commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1349529818
Your new test is running in the SQL Precommit and passing: https://ci-beam.apache.org/job/beam_PreCommit_SQL_Commit/5807/testReport/org.apache.beam.sdk.extensions.sql.zetasql/ZetaSqlDialectSpecTest/testCoalesceNotNull/
Beam ZetaSQL has significant issues with null handling, so an issue like this existing wouldn't surprise me. I think you need a schema that supports nullable fields though? Try this:
`final Schema schema = Schema.builder().addNullableField("field1", FieldType.STRING).build();`
The latest release of Dataflow SQL is based on df6efe3644d08bda747d9d4434ab9e033073c8de (somewhere between v2.38.0 and v2.39.0), which is several months old so it is possible that it has been fixed since. I know #17387 is more recent, which fixes some null bugs. Possibly try rebasing on that commit?
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1592971515
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
damccorm commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1319949862
From the conversation in https://issues.apache.org/jira/browse/BEAM-7610 and related PRs, this should have been fixed by https://github.com/apache/beam/pull/14146
@apilloud could you take a look at this one since you have more context here?
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] commented on pull request #24249: `COALESCE` should infer correct non-null return type
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #24249:
URL: https://github.com/apache/beam/pull/24249#issuecomment-1319996724
# [Codecov](https://codecov.io/gh/apache/beam/pull/24249?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#24249](https://codecov.io/gh/apache/beam/pull/24249?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cf56af2) into [master](https://codecov.io/gh/apache/beam/commit/cf56af2799f61dd4fc02923e7f23f421723765c6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cf56af2) will **not change** coverage.
> The diff coverage is `n/a`.
> :exclamation: Current head cf56af2 differs from pull request most recent head 7550cae. Consider uploading reports for the commit 7550cae to get more accurate results
```diff
@@ Coverage Diff @@
## master #24249 +/- ##
=======================================
Coverage 73.46% 73.46%
=======================================
Files 714 714
Lines 96497 96497
=======================================
Hits 70889 70889
Misses 24286 24286
Partials 1322 1322
```
| Flag | Coverage Δ | |
|---|---|---|
| go | `51.45% <0.00%> (ø)` | |
| python | `83.17% <0.00%> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org