regenerate arrow-ipc/src/gen with patched flatbuffers #6426

bkietz · 2024-09-20T15:10:45Z

Which issue does this PR close?

Closes #5052

What changes are included in this PR?

This PR regenerates using patched codegen in google/flatbuffers#8398

arrow-ipc/regen.sh is extended to
- If the $FLATC environment variable is defined, that will be used instead of building flatc from source (useful when testing against patched codegen)
- Prefix the import flatbuffers::PushAlignment to all generated files for reference by impls of Push::alignment
Endianness::equals_to_target_endianness() is moved to arrow-ipc/src/lib.rs (this was originally added to arrow-ipc/src/gen/Schema.rs manually in Result into error in case of endianness mismatches #5301)
regen.sh was rerun, cargo doc and cargo test didn't error locally

Are there any user-facing changes?

no

arrow-ipc/Cargo.toml

bkietz · 2024-09-20T15:15:08Z

arrow-ipc/regen.sh

-    sed -i '' '/use core::mem;/d' $f
-    sed -i '' '/use core::cmp::Ordering;/d' $f
-    sed -i '' '/use self::flatbuffers::{EndianScalar, Follow};/d' $f
+    sed --in-place='' '/extern crate flatbuffers;/d' $f


I don't know why but bash/sed produced an error for me until I made this substitution

sed: can't read /extern crate flatbuffers;/d: No such file or directory

Reading the man page, it seems like the expected invocation for the short flag is -ibackup, equivalent to --inplace=backup (or -i, equivalent to --inplace='')

-i[SUFFIX], --in-place[=SUFFIX] edit files in place (makes backup if SUFFIX supplied)

Should I replace this with

Suggested change

sed --in-place='' '/extern crate flatbuffers;/d' $f

sed -i '/extern crate flatbuffers;/d' $f

bkietz · 2024-09-20T15:37:57Z

arrow-ipc/regen.sh


    # Some files need prefixes
    if [[ $f == "File.rs" ]]; then 
        # Now prefix the file with the static contents
        echo -e "${PREFIX}" "${SCHEMA_IMPORT}" | cat - $f > temp && mv temp $f
    elif [[ $f == "Message.rs" ]]; then
+        sed --in-place='' 's/List<Int16>/\`List<Int16>\`/g' $f


I think this was another manual edit of the generated files. Moving it here should simplify the next time regen.sh is used. Are there any other manual edits which I haven't noticed?

arrow-ipc/Cargo.toml

bkietz · 2024-09-20T15:46:21Z

I suspect the flight failures are due to the slight increase in size due to padding FieldNode and Buffer vectors for alignment; the overages are all exactly 8 bytes greater than allowed. May I just increase the allowance?

bkietz · 2024-09-20T15:48:25Z

@tustvold @evgenyx00 could you verify this fixes the issue before we start asking for attention on google/flatbuffers?

alamb · 2024-09-25T23:01:46Z

Perhaps we could re-enable the test for nanoarrow that was disabled in #6449 as part of this PR?

alamb · 2024-09-26T20:07:25Z

.github/workflows/integration.yml

@@ -65,8 +65,7 @@ jobs:
      ARROW_INTEGRATION_JAVA: ON
      ARROW_INTEGRATION_JS: ON
      ARCHERY_INTEGRATION_TARGET_IMPLEMENTATIONS: "rust"
-      # Disable nanoarrow integration, due to https://github.com/apache/arrow-rs/issues/5052


Removing this line and having the test run successfully is quite compelling. Nice work @bkietz

arrow-ipc/regen.sh

bkietz · 2024-11-04T19:20:18Z

@alamb Is there anything which needs to happen to this PR?

tustvold · 2024-11-04T19:22:06Z

arrow-ipc/Cargo.toml

@@ -39,7 +39,7 @@ arrow-buffer = { workspace = true }
 arrow-cast = { workspace = true }
 arrow-data = { workspace = true }
 arrow-schema = { workspace = true }
-flatbuffers = { version = "24.3.25", default-features = false }
+flatbuffers = { default-features = false, git = "https://github.com/google/flatbuffers.git", rev = "49061f8c7c99363eeea25c5e4337ebb499928467" }


I think this would need to be updated to a non-git version prior to merge, otherwise we'd be unable to make a release off main.

In fact I think we can't actually release a fix for arrow with this fix as the upstream change adds a new method alignment -- it doesn't just update the generated code I think

When I reverted this pin it fails to compile

error[E0433]: failed to resolve: could not find `PushAlignment` in `flatbuffers` --> arrow-ipc/src/gen/File.rs:71:22 | 71 | flatbuffers::PushAlignment::new(8) | ^^^^^^^^^^^^^ could not find `PushAlignment` in `flatbuffers`

Thus I think we need to wait for the next flatbuffers release 🤔

alamb

Thanks @bkietz -- I think I mostly need about 4 extra hours each day !

alamb · 2024-11-04T20:18:12Z

arrow-ipc/Cargo.toml

@@ -39,7 +39,7 @@ arrow-buffer = { workspace = true }
 arrow-cast = { workspace = true }
 arrow-data = { workspace = true }
 arrow-schema = { workspace = true }
-flatbuffers = { version = "24.3.25", default-features = false }
+flatbuffers = { default-features = false, git = "https://github.com/google/flatbuffers.git", rev = "49061f8c7c99363eeea25c5e4337ebb499928467" }


In fact I think we can't actually release a fix for arrow with this fix as the upstream change adds a new method alignment -- it doesn't just update the generated code I think

alamb · 2024-11-04T20:23:29Z

arrow-ipc/Cargo.toml

@@ -39,7 +39,7 @@ arrow-buffer = { workspace = true }
 arrow-cast = { workspace = true }
 arrow-data = { workspace = true }
 arrow-schema = { workspace = true }
-flatbuffers = { version = "24.3.25", default-features = false }
+flatbuffers = { default-features = false, git = "https://github.com/google/flatbuffers.git", rev = "49061f8c7c99363eeea25c5e4337ebb499928467" }


When I reverted this pin it fails to compile

error[E0433]: failed to resolve: could not find `PushAlignment` in `flatbuffers` --> arrow-ipc/src/gen/File.rs:71:22 | 71 | flatbuffers::PushAlignment::new(8) | ^^^^^^^^^^^^^ could not find `PushAlignment` in `flatbuffers`

Thus I think we need to wait for the next flatbuffers release 🤔

osipovartem · 2024-12-16T10:06:07Z

Hey! Any updates here? 🙏

alamb · 2024-12-16T17:19:58Z

Hey! Any updates here? 🙏

The fix has been merged upstream in flatbuffers use ALIGN for Push::alignment in struct types google/flatbuffers#8398
It appears flatbuffers has not made a new release that includes the fix yet: https://crates.io/crates/flatbuffers/versions (most recent version is https://crates.io/crates/flatbuffers/24.3.25, released 8 months ago)

So maybe someone could file / find a ticket upstream in flatbuffers asking about a new rust release?

crepererum · 2024-12-18T12:38:24Z

I've pinged a potential person here:
google/flatbuffers#8398 (comment)

Let's see. flatbuffers isn't a particularly active project.

osipovartem · 2024-12-26T09:01:05Z

New flatbuffers version has been released (24.12.23)

github-actions bot added the arrow Changes to the arrow crate label Sep 20, 2024

bkietz commented Sep 20, 2024

View reviewed changes

bkietz mentioned this pull request Sep 20, 2024

Looking for help maintaining (reviewing PRs) in arrow/parquet/object_store #6418

Open

bkietz commented Sep 20, 2024

View reviewed changes

bkietz mentioned this pull request Sep 25, 2024

Fix CI by disabling newly failing rust <> nanoarrow integration test in CI #6449

Merged

alamb mentioned this pull request Sep 25, 2024

Interoperability between arrow-rs and nanoarrow #5052

Open

bkietz mentioned this pull request Sep 25, 2024

use ALIGN for Push::alignment in struct types google/flatbuffers#8398

Merged

github-actions bot added the arrow-flight Changes to the arrow-flight crate label Sep 25, 2024

bkietz force-pushed the regen-flatbuffers-with-push-alignment branch from 41d3a5b to 7c8324f Compare September 26, 2024 16:14

alamb reviewed Sep 26, 2024

View reviewed changes

paleolimbot mentioned this pull request Oct 2, 2024

Integration workflow is failing for Rust and nanoarrow apache/arrow-nanoarrow#641

Closed

bkietz added 6 commits October 25, 2024 10:07

regenerate arrow-ipc/src/gen with patched flatbuffers

9e92eff

use git repo instead of local path

7199304

add backticks

9fd99ce

expand allowed overage to accommodate more alignment padding

16fb310

re-enable nanoarrow integration test

043856d

add assertions that struct alignment is correct

8b19902

bkietz force-pushed the regen-flatbuffers-with-push-alignment branch from 7c8324f to 8b19902 Compare October 25, 2024 16:57

bkietz marked this pull request as ready for review October 25, 2024 16:58

bkietz commented Oct 25, 2024

View reviewed changes

arrow-ipc/regen.sh Outdated Show resolved Hide resolved

bkietz added 4 commits October 25, 2024 12:14

remove struct alignment assertions

5d89e5a

apply a patch to generated code rather than requiring patched flatc

43296c7

point to google/flatbuffers with pub PushAlignment

1858690

add license header to gen.patch

16783bc

bkietz requested a review from alamb October 28, 2024 21:07

tustvold reviewed Nov 4, 2024

View reviewed changes

alamb reviewed Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

regenerate arrow-ipc/src/gen with patched flatbuffers #6426

regenerate arrow-ipc/src/gen with patched flatbuffers #6426

bkietz commented Sep 20, 2024 •

edited

Loading

bkietz Sep 20, 2024

bkietz Sep 20, 2024

bkietz commented Sep 20, 2024

bkietz commented Sep 20, 2024

alamb commented Sep 25, 2024

alamb Sep 26, 2024 •

edited

Loading

bkietz commented Nov 4, 2024

tustvold Nov 4, 2024

alamb Nov 4, 2024

alamb Nov 4, 2024

alamb left a comment

alamb Nov 4, 2024

alamb Nov 4, 2024

osipovartem commented Dec 16, 2024

alamb commented Dec 16, 2024

crepererum commented Dec 18, 2024

osipovartem commented Dec 26, 2024

	sed --in-place='' '/extern crate flatbuffers;/d' $f
	sed -i '/extern crate flatbuffers;/d' $f

regenerate arrow-ipc/src/gen with patched flatbuffers #6426

Are you sure you want to change the base?

regenerate arrow-ipc/src/gen with patched flatbuffers #6426

Conversation

bkietz commented Sep 20, 2024 • edited Loading

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

bkietz Sep 20, 2024

Choose a reason for hiding this comment

bkietz Sep 20, 2024

Choose a reason for hiding this comment

bkietz commented Sep 20, 2024

bkietz commented Sep 20, 2024

alamb commented Sep 25, 2024

alamb Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

bkietz commented Nov 4, 2024

tustvold Nov 4, 2024

Choose a reason for hiding this comment

alamb Nov 4, 2024

Choose a reason for hiding this comment

alamb Nov 4, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 4, 2024

Choose a reason for hiding this comment

alamb Nov 4, 2024

Choose a reason for hiding this comment

osipovartem commented Dec 16, 2024

alamb commented Dec 16, 2024

crepererum commented Dec 18, 2024

osipovartem commented Dec 26, 2024

bkietz commented Sep 20, 2024 •

edited

Loading

alamb Sep 26, 2024 •

edited

Loading