Skip to content

Conversation

luoyuxia
Copy link
Contributor

@luoyuxia luoyuxia commented Apr 24, 2022

What is the purpose of the change

To make Calcite's varbinary type convert to Hive's binary type instead of defualt void type which is not supported in Flink.

Brief change log

  • Match varbinary type in HiveParserTypeConverter#convertPrimitiveType and convert it to Hive's binary type

Verifying this change

Additional query in HiveDialectQueryITCase#testAdditionalQueries.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? NA

@flinkbot
Copy link
Collaborator

flinkbot commented Apr 24, 2022

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@luoyuxia luoyuxia changed the title [FLINK-27304][hive] Hive's BINARY type should be converted to Flink's BINARY type instead of BYTES [FLINK-27304][hive] Calcite's varbinary type should be converted to Hive's binary type. Apr 25, 2022
@luoyuxia
Copy link
Contributor Author

@beyond1920 Could you please help review?

return hiveShim.getIntervalDayTimeTypeInfo();
case BINARY:
case VARBINARY:
return TypeInfoFactory.binaryTypeInfo;
Copy link
Contributor

@Myracle Myracle May 16, 2022

Choose a reason for hiding this comment

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

I find that there exist other types which are not present in flink code, for example, REAL, TIME_WITH_LOCAL_TIME_ZONE. Should we also add them?

Copy link
Contributor Author

@luoyuxia luoyuxia May 20, 2022

Choose a reason for hiding this comment

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

Thanks for your reminer.
Yes, we also should all them. And we have also a jira for TIME_WITH_LOCAL_TIME_ZONE FLINK-23224.
I will support it in other pr.

zhuyufeng0809 pushed a commit to zhuyufeng0809/flink that referenced this pull request Jul 1, 2022
zstraw pushed a commit to zstraw/flink that referenced this pull request Jul 4, 2022
ericccarlson pushed a commit to ericccarlson/flink that referenced this pull request Jul 11, 2022
liujiawinds pushed a commit to liujiawinds/flink that referenced this pull request Jul 22, 2022
MartijnVisser pushed a commit to sunshineJK/flink that referenced this pull request Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants