Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] Skyhook integration is broken after flatbuffers upgrade #38401

Closed
felipecrv opened this issue Oct 23, 2023 · 3 comments · Fixed by #38405
Closed

[C++] Skyhook integration is broken after flatbuffers upgrade #38401

felipecrv opened this issue Oct 23, 2023 · 3 comments · Fixed by #38405
Assignees
Milestone

Comments

@felipecrv
Copy link
Contributor

Describe the bug, including details regarding any error messages, version, and platform.

https://github.com/ursacomputing/crossbow/actions/runs/6502407264/job/17661381250#step:6:2369

FAILED: src/skyhook/CMakeFiles/arrow_skyhook_objlib.dir/protocol/skyhook_protocol.cc.o 
/usr/local/bin/sccache /usr/bin/c++  -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_MIMALLOC -DARROW_NO_DEPRECATED_API -DARROW_WITH_RE2 -DARROW_WITH_TIMING_TESTS -DARROW_WITH_UTF8PROC -DBOOST_ALL_NO_LIB -Isrc -I/arrow/cpp/src -I/arrow/cpp/src/generated -isystem /arrow/cpp/thirdparty/flatbuffers/include -isystem /arrow/cpp/thirdparty/hadoop/include -isystem google_cloud_cpp_ep-install/include -isystem absl_ep-install/include -isystem crc32c_ep-install/include -isystem orc_ep-install/include -isystem protobuf_ep-install/include -isystem awssdk_ep-install/include -isystem opentelemetry_ep-install/include -isystem xsimd_ep/src/xsimd_ep-install/include -isystem jemalloc_ep-prefix/src -isystem mimalloc_ep/src/mimalloc_ep/include/mimalloc-2.0 -Wno-noexcept-type  -fdiagnostics-color=always  -Wall -Wno-conversion -Wno-sign-conversion -Wunused-result -Wdate-time -fno-semantic-interposition -msse4.2  -g -Werror -O0 -ggdb -g1 -fPIC   -pthread -std=c++17 -MD -MT src/skyhook/CMakeFiles/arrow_skyhook_objlib.dir/protocol/skyhook_protocol.cc.o -MF src/skyhook/CMakeFiles/arrow_skyhook_objlib.dir/protocol/skyhook_protocol.cc.o.d -o src/skyhook/CMakeFiles/arrow_skyhook_objlib.dir/protocol/skyhook_protocol.cc.o -c /arrow/cpp/src/skyhook/protocol/skyhook_protocol.cc
In file included from /arrow/cpp/src/skyhook/protocol/skyhook_protocol.cc:21:
/arrow/cpp/src/skyhook/protocol/ScanRequest_generated.h: In member function 'bool org::apache::arrow::flatbuf::ScanRequest::Verify(arrow_vendored_private::flatbuffers::Verifier&) const':
/arrow/cpp/src/skyhook/protocol/ScanRequest_generated.h:45:55: error: no matching function for call to 'org::apache::arrow::flatbuf::ScanRequest::VerifyField<int64_t>(arrow_vendored_private::flatbuffers::Verifier&, org::apache::arrow::flatbuf::ScanRequest::FlatBuffersVTableOffset) const'
   45 |            VerifyField<int64_t>(verifier, VT_FILE_SIZE) &&
      |                                                       ^
In file included from /arrow/cpp/thirdparty/flatbuffers/include/flatbuffers/flatbuffer_builder.h:42,
                 from /arrow/cpp/thirdparty/flatbuffers/include/flatbuffers/flatbuffers.h:35,
                 from /arrow/cpp/src/skyhook/protocol/skyhook_protocol.cc:19:
/arrow/cpp/thirdparty/flatbuffers/include/flatbuffers/table.h:132:8: note: candidate: 'bool arrow_vendored_private::flatbuffers::Table::VerifyField(const arrow_vendored_private::flatbuffers::Verifier&, arrow_vendored_private::flatbuffers::voffset_t, size_t) const [with T = long int; arrow_vendored_private::flatbuffers::voffset_t = short unsigned int; size_t = long unsigned int]'
  132 |   bool VerifyField(const Verifier &verifier, voffset_t field,
      |        ^~~~~~~~~~~
/arrow/cpp/thirdparty/flatbuffers/include/flatbuffers/table.h:132:8: note:   candidate expects 3 arguments, 2 provided
In file included from /arrow/cpp/src/skyhook/protocol/skyhook_protocol.cc:21:
/arrow/cpp/src/skyhook/protocol/ScanRequest_generated.h:46:57: error: no matching function for call to 'org::apache::arrow::flatbuf::ScanRequest::VerifyField<int16_t>(arrow_vendored_private::flatbuffers::Verifier&, org::apache::arrow::flatbuf::ScanRequest::FlatBuffersVTableOffset) const'
   46 |            VerifyField<int16_t>(verifier, VT_FILE_FORMAT) &&
      |                                                         ^
In file included from /arrow/cpp/thirdparty/flatbuffers/include/flatbuffers/flatbuffer_builder.h:42,
                 from /arrow/cpp/thirdparty/flatbuffers/include/flatbuffers/flatbuffers.h:35,
                 from /arrow/cpp/src/skyhook/protocol/skyhook_protocol.cc:19:
/arrow/cpp/thirdparty/flatbuffers/include/flatbuffers/table.h:132:8: note: candidate: 'bool arrow_vendored_private::flatbuffers::Table::VerifyField(const arrow_vendored_private::flatbuffers::Verifier&, arrow_vendored_private::flatbuffers::voffset_t, size_t) const [with T = short int; arrow_vendored_private::flatbuffers::voffset_t = short unsigned int; size_t = long unsigned int]'
  132 |   bool VerifyField(const Verifier &verifier, voffset_t field,
      |        ^~~~~~~~~~~
/arrow/cpp/thirdparty/flatbuffers/include/flatbuffers/table.h:132:8: note:   candidate expects 3 arguments, 2 provided

Reported by @kou here #38192 (comment)

Component(s)

C++

@felipecrv
Copy link
Contributor Author

@kou should I do the same for ./cpp/src/arrow/ipc/feather.fbs in the PR or open a separate issue?

@kou
Copy link
Member

kou commented Oct 23, 2023

It seems that feather.fbs was already re-generated in #38192: https://github.com/apache/arrow/pull/38192/files#diff-b078499b73cc90e53b2a0f759c7d5374e5f61f67f714baf33ee6bf07c8aa61c1

Our update script includes feather.fbs:

FILES+=("$SOURCE_DIR/arrow/ipc/feather.fbs")

kou pushed a commit that referenced this issue Oct 23, 2023
### Rationale for this change

Vendored flatbuffer headers have been upgraded so the checked-in flatc-generated files need to be upgraded.

### What changes are included in this PR?

 - Changes to the shell script that re-generates flatbuffers
 - Check-in of the newly generated `ScanRequest_generated.h`

### Are these changes tested?

By integration tests.
* Closes: #38401

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 15.0.0 milestone Oct 23, 2023
@felipecrv
Copy link
Contributor Author

It seems that feather.fbs was already re-generated in #38192: https://github.com/apache/arrow/pull/38192/files#diff-b078499b73cc90e53b2a0f759c7d5374e5f61f67f714baf33ee6bf07c8aa61c1

Our update script includes feather.fbs:

FILES+=("$SOURCE_DIR/arrow/ipc/feather.fbs")

Right! I changed the script to include the skyhook file and didn't notice that.

JerAguilon pushed a commit to JerAguilon/arrow that referenced this issue Oct 25, 2023
…#38405)

### Rationale for this change

Vendored flatbuffer headers have been upgraded so the checked-in flatc-generated files need to be upgraded.

### What changes are included in this PR?

 - Changes to the shell script that re-generates flatbuffers
 - Check-in of the newly generated `ScanRequest_generated.h`

### Are these changes tested?

By integration tests.
* Closes: apache#38401

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…#38405)

### Rationale for this change

Vendored flatbuffer headers have been upgraded so the checked-in flatc-generated files need to be upgraded.

### What changes are included in this PR?

 - Changes to the shell script that re-generates flatbuffers
 - Check-in of the newly generated `ScanRequest_generated.h`

### Are these changes tested?

By integration tests.
* Closes: apache#38401

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…#38405)

### Rationale for this change

Vendored flatbuffer headers have been upgraded so the checked-in flatc-generated files need to be upgraded.

### What changes are included in this PR?

 - Changes to the shell script that re-generates flatbuffers
 - Check-in of the newly generated `ScanRequest_generated.h`

### Are these changes tested?

By integration tests.
* Closes: apache#38401

Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants