GH-38053: [C++][Go] Re-generate sources from Schema.fbs #38054

felipecrv · 2023-10-05T19:47:37Z

Rationale for this change

Re-generate Go and C++ sources from Flatbuffers specs now including string-view and list-view types.

What changes are included in this PR?

Documentation fixes on the .fbs files and the generated C++ and Go source files.

Are these changes tested?

The files should be correct by construction. Existing tests guarantee that no mistake was made in re-generating the files.

Closes: [Go][C++] Re-generate flatbuffer sources #38053

github-actions · 2023-10-05T19:48:12Z

⚠️ GitHub issue #38053 has been automatically assigned in GitHub to PR creator.

bkietz · 2023-10-06T15:32:55Z

Unfortunately, a string matching /<a\b/ can't be used in comments in flatbuffer schemas (since the comments are copied to the generated c++, where they are recognized as ill formed html). I corrected this in my PR but if you're going to regenerate here then you'll need to modify Message.fbs

https://github.com/apache/arrow/pull/37792/files#diff-0623d567d0260222d5501b4e169141b5070eabc2ec09c3482da453a3346c5bf3R109

felipecrv · 2023-10-06T15:37:47Z

Thank you @bkietz. I pushed the fixes now and regenerated the sources again.

zeroshade

All looks good to me from a Go standpoint, i'll wait for someone to approve in addition before merging this

pitrou · 2023-10-09T09:20:15Z

cpp/src/generated/Tensor_generated.h

@@ -59,6 +59,7 @@ struct TensorDimBuilder {
        : fbb_(_fbb) {
    start_ = fbb_.StartTable();
  }
+  TensorDimBuilder &operator=(const TensorDimBuilder &);


These declarations don't seem to cause any problem, but they are weird nevertheless, as the corresponding definitions do not exist. Which version of Flatbuffers did you use?

Using flatc version 23.5.26 (the latest release), I don't get these declarations.

I used version 1.12.0 which is the version that generates the smallest diff and causes the least trouble.

pitrou · 2023-10-09T09:25:41Z

Regardless of which flatc version is being used, it seems we should perhaps upgrade the vendored flatbuffers headers to the same version.

For example, flatc 23.5.26 generates the following version check which seems to imply that the versions should match:

// Ensure the included flatbuffers.h is the same version as when this file was
// generated, otherwise it may not be compatible.
static_assert(FLATBUFFERS_VERSION_MAJOR == 23 &&
              FLATBUFFERS_VERSION_MINOR == 5 &&
              FLATBUFFERS_VERSION_REVISION == 26,
             "Non-compatible flatbuffers version included");

pitrou · 2023-10-09T09:59:24Z

@github-actions crossbow submit -g nightly-tests

github-actions · 2023-10-09T10:03:12Z

Revision: 49b424b

Submitted crossbow builds: ursacomputing/crossbow @ actions-66dcf81e0b

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-alpine-linux-cpp
test-build-cpp-fuzz
test-build-vcpkg-win
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-go-1.17
test-debian-11-go-1.20
test-debian-11-python-3
test-debian-c-glib
test-debian-ruby
test-fedora-35-cpp
test-fedora-35-python-3
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-library-r-base-latest
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-skyhook-integration
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-docs
test-ubuntu-22.04-python-3
test-ubuntu-c-glib
test-ubuntu-r-sanitizer
test-ubuntu-ruby

pitrou · 2023-10-09T13:26:21Z

Regardless of which flatc version is being used, it seems we should perhaps upgrade the vendored flatbuffers headers to the same version.

That said, as long as CI doesn't complain, we can merge and bump Flatbuffers later.

felipecrv · 2023-10-09T13:47:34Z

Regardless of which flatc version is being used, it seems we should perhaps upgrade the vendored flatbuffers headers to the same version.

That said, as long as CI doesn't complain, we can merge and bump Flatbuffers later.

The R-related failures seem unrelated.

pitrou · 2023-10-09T14:05:06Z

Agreed, I think we can merge now.

conbench-apache-arrow · 2023-10-10T14:47:19Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit d353826.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.

…#38054) ### Rationale for this change Re-generate Go and C++ sources from Flatbuffers specs now including string-view and list-view types. ### What changes are included in this PR? Documentation fixes on the .fbs files and the generated C++ and Go source files. ### Are these changes tested? The files should be correct by construction. Existing tests guarantee that no mistake was made in re-generating the files. * Closes: apache#38053 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

felipecrv requested a review from zeroshade as a code owner October 5, 2023 19:47

github-actions bot added Component: Go Component: C++ awaiting review Awaiting review labels Oct 5, 2023

felipecrv requested a review from bkietz October 5, 2023 19:51

felipecrv force-pushed the flatbuffer_go_cpp branch 2 times, most recently from f21f3a9 to 22d521f Compare October 6, 2023 19:06

zeroshade approved these changes Oct 6, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Oct 6, 2023

felipecrv added 3 commits October 9, 2023 11:08

format: Two documentation fixes to the .fbs files

8b85958

Go: Re-generate from Schema.fbs

07b8394

C++: Re-generate from Schema.fbs

49b424b

pitrou force-pushed the flatbuffer_go_cpp branch from 22d521f to 49b424b Compare October 9, 2023 09:10

pitrou reviewed Oct 9, 2023

View reviewed changes

bkietz approved these changes Oct 9, 2023

View reviewed changes

pitrou merged commit d353826 into apache:main Oct 9, 2023
48 of 50 checks passed

pitrou removed the awaiting merge Awaiting merge label Oct 9, 2023

felipecrv deleted the flatbuffer_go_cpp branch October 9, 2023 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38053: [C++][Go] Re-generate sources from Schema.fbs #38054

GH-38053: [C++][Go] Re-generate sources from Schema.fbs #38054

felipecrv commented Oct 5, 2023 •

edited

Loading

github-actions bot commented Oct 5, 2023

bkietz commented Oct 6, 2023

felipecrv commented Oct 6, 2023

zeroshade left a comment

pitrou Oct 9, 2023

felipecrv Oct 9, 2023

pitrou commented Oct 9, 2023

pitrou commented Oct 9, 2023

github-actions bot commented Oct 9, 2023

pitrou commented Oct 9, 2023

felipecrv commented Oct 9, 2023

pitrou commented Oct 9, 2023

conbench-apache-arrow bot commented Oct 10, 2023

GH-38053: [C++][Go] Re-generate sources from Schema.fbs #38054

GH-38053: [C++][Go] Re-generate sources from Schema.fbs #38054

Conversation

felipecrv commented Oct 5, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

github-actions bot commented Oct 5, 2023

bkietz commented Oct 6, 2023

felipecrv commented Oct 6, 2023

zeroshade left a comment

Choose a reason for hiding this comment

pitrou Oct 9, 2023

Choose a reason for hiding this comment

felipecrv Oct 9, 2023

Choose a reason for hiding this comment

pitrou commented Oct 9, 2023

pitrou commented Oct 9, 2023

github-actions bot commented Oct 9, 2023

pitrou commented Oct 9, 2023

felipecrv commented Oct 9, 2023

pitrou commented Oct 9, 2023

conbench-apache-arrow bot commented Oct 10, 2023

felipecrv commented Oct 5, 2023 •

edited

Loading