Skip to content

Backport GooglePolyline and Parquet writer fixes to branch-1.7 - #2298

Merged
FelixYBW merged 3 commits into
IBM:branch-1.7-enhancedfrom
philo-he:backport-patch-enhanced
Jul 27, 2026
Merged

Backport GooglePolyline and Parquet writer fixes to branch-1.7#2298
FelixYBW merged 3 commits into
IBM:branch-1.7-enhancedfrom
philo-he:backport-patch-enhanced

Conversation

@philo-he

@philo-he philo-he commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Backported the folowing Velox commits, with code conflicts resolved.

Gluten test PR: apache/gluten#12561

@prashantgolash @philo-he
fix(geo): Gate GooglePolyline on VELOX_ENABLE_GEO (facebookincubator#…
2189f26 
…17850)

Summary:
Pull Request resolved: facebookincubator#17850

[[Nimble build break](https://github.com/facebookincubator/nimble/actions/runs/27595278029/job/81584210938?pr=876)]

When the google_polyline functions were added D107672113, GooglePolylineFunctions.cpp was placed in the unconditional source list of velox_functions_prestosql_impl, but its header pulls in GEOS, which is only fetched and added to the include path when VELOX_ENABLE_GEO is on. The test sources and the function registration were correctly gated behind VELOX_ENABLE_GEO, but the production compile unit was left in the unconditional list. Move it into the existing if(VELOX_ENABLE_GEO) block, using velox_sources so it routes correctly in mono-library builds, so a GEO=OFF build no longer compiles it.

Reviewed By: xiaoxmeng

Differential Revision: D108714293

fbshipit-source-id: 592cb824e99052e8a4d73ca7acd2658b856ba9cb
@philo-he
philo-he requested a review from majetideepak as a code owner July 21, 2026 08:10
@philo-he
philo-he changed the base branch from main to branch-1.7-enhanced July 21, 2026 08:11
@philo-he
philo-he force-pushed the backport-patch-enhanced branch from 8364a92 to 1d6b3e9 Compare July 22, 2026 18:24
@wecharyu @philo-he
fix(parquet): Flush row group by buffered bytes in the writer
12b2a8f 
Differential Revision: D111035585

Pull Request resolved: facebookincubator#16998
@philo-he
philo-he force-pushed the backport-patch-enhanced branch from 1d6b3e9 to 12b2a8f Compare July 23, 2026 19:15
@philo-he

philo-he commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Gluten CI passed: apache/gluten#12561

Also verified the following Velox parequet tests, no test failure.

velox_parquet_writer_test
velox_dwio_parquet_reader_test
velox_dwio_arrow_parquet_writer_test

@philo-he philo-he changed the title Backport patch enhanced Backport GeometryToGooglePolyline and Parquet writer fixes to branch-1.7 Jul 26, 2026
@philo-he philo-he changed the title Backport GeometryToGooglePolyline and Parquet writer fixes to branch-1.7 Backport GooglePolyline and Parquet writer fixes to branch-1.7 Jul 26, 2026
@philo-he

philo-he commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

@zhouyuan, could you take a look? cc @FelixYBW

@FelixYBW

FelixYBW commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Thank you so much, @philo-he

@FelixYBW
FelixYBW merged commit dd3f64a into IBM:branch-1.7-enhanced Jul 27, 2026
4 of 6 checks passed
@zhouyuan

zhouyuan commented Jul 27, 2026

Copy link
Copy Markdown
Member

@philo-he GEO related components is not enabled in gluten https://github.com/apache/gluten/blob/main/ep/build-velox/src/build-velox.sh#L129

@FelixYBW

FelixYBW commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Is it requested by your customer?

@philo-he

philo-he commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

@zhouyuan @FelixYBW, let me clarify. For the cherry-picked commit fix(geo): Gate GooglePolyline on VELOX_ENABLE_GEO: it simply moves one C++ file under the VELOX_ENABLE_GEO guard. Without it, when VELOX_ENABLE_GEO is disabled and the GEOS library isn't installed, the build fails with a missing-header error. In our Gluten build, we do disable VELOX_ENABLE_GEO, but the build environment happens to have GEOS installed, so we don't hit the error. However, a user who doesn't care about GEO—disabling VELOX_ENABLE_GEO without installing the GEOS lib—would run into the build failure.

@FelixYBW

FelixYBW commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Thank you for your clarification. @philo-he. Let's add this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FelixYBW FelixYBW FelixYBW approved these changes
@majetideepak majetideepak Awaiting requested review from majetideepak majetideepak is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@philo-he @FelixYBW @zhouyuan @prashantgolash @wecharyu