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

IIRC Mac's sed (BSD utils) and the Linux coreutils sed behave differently. It's a bit of a pain.

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)

alamb · 2024-12-30T11:10:01Z

New flatbuffers version has been released (24.12.23)

Thanks @osipovartem

@osipovartem or @bkietz -- can one of you prepare a PR using the newly released flatbuffers code? I would be happy to review it afterwards.

alamb

This looks great (and epic) to me. Thank you so much @bkietz for pushing it along and sticking with it 🎉 🚀 🏅

alamb · 2025-01-06T20:57:08Z

arrow-ipc/Cargo.toml

@@ -38,7 +38,7 @@ arrow-array = { workspace = true }
 arrow-buffer = { workspace = true }
 arrow-data = { workspace = true }
 arrow-schema = { workspace = true }
-flatbuffers = { version = "24.3.25", default-features = false }
+flatbuffers = { version = "24.12.23", default-features = false }


alamb · 2025-01-06T20:59:16Z

arrow-ipc/src/gen/Schema.rs

@@ -1057,15 +1057,6 @@ impl Endianness {
            _ => None,
        }
    }
-
-    /// Returns true if the endianness of the source system matches the endianness of the target system.


💯 for extracting this out to a non gen file

crepererum · 2025-01-07T16:00:17Z

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


IIRC Mac's sed (BSD utils) and the Linux coreutils sed behave differently. It's a bit of a pain.

crepererum · 2025-01-07T16:02:07Z

arrow-ipc/gen.patch

+         let src = ::core::slice::from_raw_parts(self as *const Block as *const u8, Self::size());
+         dst.copy_from_slice(src);
+     }
+    #[inline]


Shouldn't flatbuffers 24.12.23 do that now?

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 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 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

Jefffrey mentioned this pull request Jan 5, 2025

Re-enable nano arrow archery integration #6599

Closed

bkietz added 3 commits January 6, 2025 09:12

regenerate arrow-ipc/src/gen with patched flatbuffers

df868b4

use git repo instead of local path

d850dec

add backticks

7fa795c

bkietz added 8 commits January 6, 2025 11:59

expand allowed overage to accommodate more alignment padding

de03d26

re-enable nanoarrow integration test

e1378c9

add assertions that struct alignment is correct

7121201

remove struct alignment assertions

a5803fb

apply a patch to generated code rather than requiring patched flatc

1dd6c28

point to google/flatbuffers with pub PushAlignment

c0c9ea8

add license header to gen.patch

d454ae0

use flatbuffers 24.12.23

ac8b01c

bkietz force-pushed the regen-flatbuffers-with-push-alignment branch from 16783bc to ac8b01c Compare January 6, 2025 20:03

alamb approved these changes Jan 6, 2025

View reviewed changes

crepererum reviewed Jan 7, 2025

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

crepererum Jan 7, 2025

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

alamb commented Dec 30, 2024

alamb left a comment

alamb Jan 6, 2025

alamb Jan 6, 2025

crepererum Jan 7, 2025

crepererum Jan 7, 2025

	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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

alamb commented Dec 30, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkietz commented Sep 20, 2024 •

edited

Loading

alamb Sep 26, 2024 •

edited

Loading