You are viewing a plain text version of this content. The canonical link for it is here.
Posted to devnull@infra.apache.org by "XuMingmin (GitHub)" <gi...@apache.org> on 2018/08/23 23:20:55 UTC
[GitHub] [beam] XuMingmin opened pull request #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
A quick bug fix to support VARBINARY in BeamSqlPrimitive.
------------
R: @apilloud @akedin @xumingming @amaliujia @vectorijk
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] apilloud commented on pull request #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "apilloud (GitHub)" <gi...@apache.org>.
You should add a `BYTES` case here too.
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] XuMingmin commented on issue #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "XuMingmin (GitHub)" <gi...@apache.org>.
retest this please
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #6270:
[BEAM-5210] Support VARBINARY in BeamSqlPrimitive
Posted by "amaliujia (GitHub)" <gi...@apache.org>.
Why not test `ByteString` as well?
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] apilloud commented on pull request #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "apilloud (GitHub)" <gi...@apache.org>.
Did this slip through on accident?
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #6270:
[BEAM-5210] Support VARBINARY in BeamSqlPrimitive
Posted by "amaliujia (GitHub)" <gi...@apache.org>.
There is no support for BINARY in `BeamSqlPrimitive.java`
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #6270:
[BEAM-5210] Support VARBINARY in BeamSqlPrimitive
Posted by "amaliujia (GitHub)" <gi...@apache.org>.
I am not getting this change. After it, it will have `BYTES` mapping to both `BINARY` and `VARBINARY`. It does not pass compilation.
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #6270:
[BEAM-5210] Support VARBINARY in BeamSqlPrimitive
Posted by "amaliujia (GitHub)" <gi...@apache.org>.
There is no `BYTES` in [SqlTypeName](https://calcite.apache.org/apidocs/org/apache/calcite/sql/type/SqlTypeName.html), `BINARY` is missed though.
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] XuMingmin commented on pull request #6270:
[BEAM-5210] Support VARBINARY in BeamSqlPrimitive
Posted by "XuMingmin (GitHub)" <gi...@apache.org>.
yes, let me remove it
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] XuMingmin commented on issue #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "XuMingmin (GitHub)" <gi...@apache.org>.
Thanks, let me squash and merge it.
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] apilloud commented on issue #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "apilloud (GitHub)" <gi...@apache.org>.
LGTM.
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] amaliujia commented on issue #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "amaliujia (GitHub)" <gi...@apache.org>.
Thanks for this fix!
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] XuMingmin commented on pull request #6270:
[BEAM-5210] Support VARBINARY in BeamSqlPrimitive
Posted by "XuMingmin (GitHub)" <gi...@apache.org>.
good point, I only considered the type in `Row`, let me update it.
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] XuMingmin commented on issue #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "XuMingmin (GitHub)" <gi...@apache.org>.
retest this please
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] apilloud commented on pull request #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "apilloud (GitHub)" <gi...@apache.org>.
Calcite is using `ByteString` for both BINARY and VARBINARY: https://calcite.apache.org/avatica/apidocs/org/apache/calcite/avatica/util/ByteString.html We should match, as that will minimize pain when I pull in the Calcite Rex code generation.
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] XuMingmin closed pull request #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "XuMingmin (GitHub)" <gi...@apache.org>.
[ pull request closed by XuMingmin ]
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #6270:
[BEAM-5210] Support VARBINARY in BeamSqlPrimitive
Posted by "amaliujia (GitHub)" <gi...@apache.org>.
There is no support for BINARY in `BeamSqlPrimitive.java`
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] XuMingmin commented on issue #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "XuMingmin (GitHub)" <gi...@apache.org>.
Update: notice compile issue with `SqlTypeName.BINARY`. I'd focus on the bug first and leave `SqlTypeName.BINARY` in future work.
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] XuMingmin commented on issue #6270: [BEAM-5210]
Support VARBINARY in BeamSqlPrimitive
Posted by "XuMingmin (GitHub)" <gi...@apache.org>.
retest this please
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [beam] XuMingmin commented on pull request #6270:
[BEAM-5210] Support VARBINARY in BeamSqlPrimitive
Posted by "XuMingmin (GitHub)" <gi...@apache.org>.
In SQL itself, `BINARY` and `VARBINARY` are not distinguished significantly. I prefer to have both mapping to `BYTES`, same as `CHAR` and `VARCHAR`.
[ Full content available at: https://github.com/apache/beam/pull/6270 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org