[native] Type mismatch build error in fromThrift() call with TimeZoneKey on MacOS

[anandamideShakyan]

Description

ProtocolToThrift.cpp fails to compile due to a missing overload of fromThrift() that accepts a TimeZoneKey type. The compiler cannot resolve the correct conversion from facebook::presto::protocol::TimeZoneKey (aka long) to any of the existing fromThrift() function signatures.

[1202/1499] Building CXX object presto_cpp/main/thrift/CMakeFiles/presto_thrift_extra.dir/ProtocolToThrift.cpp.o
FAILED: presto_cpp/main/thrift/CMakeFiles/presto_thrift_extra.dir/ProtocolToThrift.cpp.o 
ccache /usr/bin/c++ -DFOLLY_CFG_NO_COROUTINES -DFOLLY_HAVE_INT128_T=1 -I/Users/shakyan/opensource/presto/presto-native-execution/. -I/Users/shakyan/opensource/presto/presto-native-execution/velox -I/Users/shakyan/opensource/presto/presto-native-execution/velox/velox/external/xxhash -I/Users/shakyan/opensource/presto/presto-native-execution/_build/debug/velox -I/Users/shakyan/opensource/presto/presto-native-execution/_build/debug -I/include -I/Users/shakyan/opensource/presto/presto-native-execution/_build/debug/presto_cpp/main/thrift -I/opt/homebrew/include -isystem /Users/shakyan/opensource/dependencies/deps-install/include -isystem /opt/homebrew/Cellar/openssl@3/3.5.0/include -isystem /Users/shakyan/opensource/dependencies/deps-install/include/proxygen -Wno-error=missing-template-arg-list-after-template-kw -Wno-error=explicit-specialization-storage-class -mcpu=apple-m1+crc -Wno-nullability-completeness -Wno-deprecated-declarations -Wreorder -Werror -g -std=gnu++17 -arch arm64 -MD -MT presto_cpp/main/thrift/CMakeFiles/presto_thrift_extra.dir/ProtocolToThrift.cpp.o -MF presto_cpp/main/thrift/CMakeFiles/presto_thrift_extra.dir/ProtocolToThrift.cpp.o.d -o presto_cpp/main/thrift/CMakeFiles/presto_thrift_extra.dir/ProtocolToThrift.cpp.o -c /Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp
/Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp:643:3: error: no matching function for call to 'fromThrift'
  643 |   fromThrift(*thrift.key_ref(), proto);
      |   ^~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp:115:6: note: candidate function not viable: no known conversion from 'facebook::presto::protocol::TimeZoneKey' (aka 'long') to 'int16_t &' (aka 'short &') for 2nd argument
  115 | void fromThrift(const int16_t& thrift, int16_t& proto) {
      |      ^                                 ~~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp:118:6: note: candidate function not viable: no known conversion from 'facebook::presto::protocol::TimeZoneKey' (aka 'long') to 'int32_t &' (aka 'int &') for 2nd argument
  118 | void fromThrift(const int32_t& thrift, int32_t& proto) {
      |      ^                                 ~~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp:112:6: note: candidate function not viable: no known conversion from 'facebook::presto::protocol::TimeZoneKey' (aka 'long') to 'bool &' for 2nd argument
  112 | void fromThrift(const bool& thrift, bool& proto) {
      |      ^                              ~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp:121:6: note: candidate function not viable: no known conversion from 'facebook::presto::protocol::TimeZoneKey' (aka 'long') to 'int64_t &' (aka 'long long &') for 2nd argument
  121 | void fromThrift(const int64_t& thrift, int64_t& proto) {
      |      ^                                 ~~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp:124:6: note: candidate function not viable: no known conversion from 'facebook::presto::protocol::TimeZoneKey' (aka 'long') to 'double &' for 2nd argument
  124 | void fromThrift(const double& thrift, double& proto) {
      |      ^                                ~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp:128:6: note: candidate function not viable: no known conversion from 'facebook::presto::protocol::TimeZoneKey' (aka 'long') to 'facebook::presto::protocol::Duration &' for 2nd argument
  128 | void fromThrift(
      |      ^
  129 |     const double& thrift,
  130 |     facebook::presto::protocol::Duration& duration) {
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp:135:6: note: candidate function not viable: no known conversion from 'facebook::presto::protocol::TimeZoneKey' (aka 'long') to 'facebook::presto::protocol::DataSize &' for 2nd argument
  135 | void fromThrift(
      |      ^
  136 |     const double& thrift,
  137 |     facebook::presto::protocol::DataSize& dataSize) {
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp:640:6: note: candidate function not viable: no known conversion from 'const short' to 'const TimeZoneKey' for 1st argument
  640 | void fromThrift(
      |      ^
  641 |     const TimeZoneKey& thrift,
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/./presto_cpp/main/thrift/ProtocolToThrift.h:87:6: note: candidate function not viable: no known conversion from 'const short' to 'const Type' for 1st argument
   87 | void fromThrift(const Type& thrift, facebook::presto::protocol::Type& proto);
      |      ^          ~~~~~~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/./presto_cpp/main/thrift/ProtocolToThrift.h:240:6: note: candidate function not viable: no known conversion from 'const short' to 'const ResourceEstimates' for 1st argument
  240 | void fromThrift(
      |      ^
  241 |     const ResourceEstimates& thrift,
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/./presto_cpp/main/thrift/ProtocolToThrift.h:247:6: note: candidate function not viable: no known conversion from 'const short' to 'const ConnectorId' for 1st argument
  247 | void fromThrift(
      |      ^
  248 |     const ConnectorId& thrift,
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/shakyan/opensource/presto/presto-native-execution/./presto_cpp/main/thrift/ProtocolToThrift.h:254:6: note: candidate function not viable: no known conversion from 'const short' to 'const SqlFunctionId' for 1st argument
  254 | void fromThrift(
      |      ^
  255 |     const SqlFunctionId& thrift,

Expected Behavior

Presto CPP should build without errors

Current Behavior

The build fails while building due to a missing

[majetideepak]

@vhsu14 can you please take a look?
Seems to be broken due to changes from #25162
CC: @amitkdutta @gggrace14

[vhsu14]

Hey @majetideepak, thanks for raising the issue. The code implementation for TimeZoneKey is basically the same as the previous PR, which passed the tests (https://github.com/prestodb/presto/actions/runs/15056229702). Curious what has changed for the test since then? I think the issue is long cannot be casted to int16/ int32/ int64, how do I check how many bits long is for the test?

[majetideepak]

@vhsu14: Looks like your original PR #25162 introduced this error but got masked by the bison build failure error in between.
Here is a run on your branch that show the same error as the one posted here.
https://github.com/prestodb/presto/actions/runs/15172439723/job/42670109651

[majetideepak]

@vhsu14 I see that presto_cpp/main/thrift/special/TimeZoneKey.cpp.inc is not included in the generated protocol.
We need

void toThrift(const facebook::presto::protocol::TimeZoneKey& proto, TimeZoneKey& thrift);
void fromThrift(const TimeZoneKey& thrift, facebook::presto::protocol::TimeZoneKey& proto);

[vhsu14]

@majetideepak thanks a lot for providing the info and looking into this! not sure if I'm understanding correctly, I believe presto_cpp/main/thrift/special/TimeZoneKey.cpp.inc is included in ProtocolToThrift.cpp#L635-L644. I think the issue might be due to TimeZoneKey being long. The failures for groupId and sequenceId were resolved by changing their types to int and int64_t respectively, but there was some correctness issue with TimeZoneKey. Do we define the bit length for long somewhere in the test?

[czentgr]

@vhsu14 The functions for fromThrift and toThrift involving the TimeZoneKey are calling the toThrift and fromThrift for built-in types again (e.g.

presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.cpp

Line 635 in c6115c3

void toThrift(

)

protocol::TimeZoneKey = long (

presto/presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h

Line 110 in c6115c3

using TimeZoneKey = long;

)
and TimeZoneKey is a struct composed of an id and key member. The latter is an i16 (aka short aka int16_t) defined here:

presto/presto-native-execution/presto_cpp/main/thrift/presto_thrift.thrift

Line 239 in c6115c3

struct TimeZoneKey {

As a result the requested signatures are

• toThrift(long, short)
• fromThrift(short, long)

But this type combination does not exist for simple types in the mustache file.
One change would be to change the presto_protocol_core.h TimeZoneKey type from long to int16_t. Then we have a requested signature of

• toThrift(int16_t, int16_t)
• fromThrift(int16_t, int16_t)

which does exist here:

presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift-cpp.mustache

Line 34 in c6115c3

void toThrift(const int16_t& proto, int16_t& thrift) {

and

presto/presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift-cpp.mustache

Line 112 in c6115c3

void fromThrift(const int16_t& thrift, int16_t& proto) {

Of course, another option is to add the to/from functions for the requested signatures.

[majetideepak]

@vhsu14 TimeZoneKey Thrift code generation seems incorrect since it is missing the id member of the struct which is important.

struct TimeZoneKey {
  1: string id;
  2: i16 key;
}