feat: Upgrade velox submodule to y-scope/velox@1604a4d to add support for CLP's `FormattedFloat` and `DictionaryFloat` types. by anlowee · Pull Request #73 · y-scope/presto

anlowee · 2025-09-30T23:35:28Z

Description

This PR adds support for FormattedFloat and DictionaryFloat types in the latest CLP. This PR also has a related PR in Velox repo: y-scope/velox#37, so this PR should be merged after that PR is merged.

Checklist

The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

End-to-end tested the queries used in the unit tests in this PR: y-scope/velox#37
Passed the CI.

Summary by CodeRabbit

New Features
- Added two additional CLP float types; both are now returned as DOUBLE for consistent numeric handling.
Documentation
- Updated connector docs with a new section describing the expanded set of CLP double types and their unified mapping to DOUBLE.
Chores
- Updated native execution submodule pointer to the latest commit for alignment.

coderabbitai · 2025-09-30T23:35:36Z

Walkthrough

Adds two CLP node types (FormattedFloat, DictionaryFloat), maps them to Presto DOUBLE in schema conversion, updates CLP docs to list the new double types, and advances the Velox submodule pointer.

Changes

Cohort / File(s)	Summary of Changes
CLP type mapping updates `presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTree.java`	Extends column type mapping to include `FormattedFloat` and `DictionaryFloat`, both mapped to Presto `DOUBLE`; switch structure and error handling unchanged.
CLP enum extensions `presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTreeNodeType.java`	Adds enum constants `FormattedFloat((byte) 12)` and `DictionaryFloat((byte) 13)` and adjusts enum list punctuation/order; no method or behavioral changes.
Documentation `presto-docs/src/main/sphinx/connector/clp.rst`	Adds "Double Types" section and updates data types table to include `FormattedFloat` and `DictionaryFloat` mapped to `DOUBLE`.
Submodule pointer `presto-native-execution/velox`	Advances Velox submodule reference from commit `7ae51198424559a7ed6d245ce4c4214ef7047d9d` to `1604a4deb95924f2f5ddabf8bffbcd32a990f7d3`; no repository code edits in this diff.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor CLP_Schema
    participant SchemaMapper as Presto Schema Mapper
    note right of CLP_Schema #DDDDFF: New CLP types added
    CLP_Schema->>SchemaMapper: send column type (Float / FormattedFloat / DictionaryFloat)
    SchemaMapper-->>CLP_Schema: map to Presto DOUBLE
    note right of SchemaMapper #EEFFEE: unified mapping to DOUBLE

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description does not follow the repository’s required template headings and is missing the Motivation and Context, Impact, Test Plan, and Release Notes sections, so it lacks the structured information expected by the template.	Please restructure the description to use the prescribed template headings (## Description, ## Motivation and Context, ## Impact, ## Test Plan, and ## Release Notes) and ensure each section is filled out with the relevant details.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Title Check	✅ Passed	The title accurately summarises the primary change by indicating the Velox submodule is upgraded to a specific commit to support the new FormattedFloat and DictionaryFloat types, which reflects the core modifications in code, enums, and documentation. It is concise, specific, and formatted as a single clear sentence, making it easy for reviewers scanning history to understand the main purpose. The inclusion of the commit reference and type names provides useful context without unnecessary detail.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 971443a and 2ccaa91.

📒 Files selected for processing (3)

presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTree.java (1 hunks)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTreeNodeType.java (1 hunks)
presto-native-execution/velox (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)

GitHub Check: test (17.0.13, :presto-tests -P ci-only-plan-determinism)
GitHub Check: test (17.0.13, :presto-main)
GitHub Check: test (17.0.13, :presto-tests -P presto-tests-general)
GitHub Check: test (17.0.13, :presto-tests -P ci-only-local-queries)
GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-queries)
GitHub Check: test (8.0.442, :presto-main-base)
GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
GitHub Check: test (8.0.442, :presto-tests -P ci-only-plan-determinism)
GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
GitHub Check: maven-checks (17.0.13)
GitHub Check: maven-checks (8.0.442)
GitHub Check: prestocpp-linux-build-for-test
GitHub Check: prestissimo-worker-images-build

🔇 Additional comments (2)

presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTreeNodeType.java (1)

30-32: Enum extension looks consistent.

The new constants integrate cleanly with the existing lookup-table initialisation, so no further adjustments are needed.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTree.java (1)

120-122: Switch mapping covers the new float variants.

Routing FormattedFloat and DictionaryFloat to DOUBLE keeps the Presto surface aligned with the legacy Float handling.

kirkrodrigues · 2025-10-03T19:25:34Z

+CLP uses three distinct double types: ``Float`` (a standard IEEE-754 double-precision value), ``FormattedFloat`` (a
+double-precision value stored together with its original formatting details), and ``DictionaryFloat`` (a
+double-precision value whose full string representation is stored in the variable dictionary, with values encoded by
+their corresponding dictionary IDs). At present, all three are mapped to Presto's ``DOUBLE`` type.


Can we format this as a list for readability?

kirkrodrigues

Approving docs.

kirkrodrigues

Edited PR title directly.

Add support for formatted float and dictionary float

2ccaa91

anlowee force-pushed the xwei/add-formatted-float branch from 96b709a to 2ccaa91 Compare September 30, 2025 23:35

coderabbitai Bot reviewed Sep 30, 2025

View reviewed changes

Comment thread presto-native-execution/velox Outdated

Update docs

068f30e

anlowee requested review from kirkrodrigues and wraymo September 30, 2025 23:53

anlowee mentioned this pull request Sep 30, 2025

feat: Upgrade clp submodule to y-scope/clp@67276c0; Add support for FormattedFloat and DictionaryFloat. y-scope/velox#37

Merged

3 tasks

Bump velox version

f74a035

anlowee changed the title ~~feat: Add support for FormattedFloat and DictionaryFloat types.~~ feat: Upgrade velox submodule to y-scope/velox@1604a4d to add support for FormattedFloat and DictionaryFloat types. Oct 3, 2025

kirkrodrigues reviewed Oct 3, 2025

View reviewed changes

Address comments

1f24947

kirkrodrigues approved these changes Oct 3, 2025

View reviewed changes

kirkrodrigues changed the title ~~feat: Upgrade velox submodule to y-scope/velox@1604a4d to add support for FormattedFloat and DictionaryFloat types.~~ feat: Upgrade velox submodule to y-scope/velox@1604a4d to add support for CLP's FormattedFloat and DictionaryFloat types. Oct 3, 2025

kirkrodrigues approved these changes Oct 3, 2025

View reviewed changes

anlowee merged commit bea15e7 into y-scope:release-0.293-clp-connector Oct 3, 2025
59 of 60 checks passed

anlowee deleted the xwei/add-formatted-float branch October 3, 2025 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Upgrade velox submodule to y-scope/velox@1604a4d to add support for CLP's `FormattedFloat` and `DictionaryFloat` types.#73

feat: Upgrade velox submodule to y-scope/velox@1604a4d to add support for CLP's `FormattedFloat` and `DictionaryFloat` types.#73
anlowee merged 4 commits into
y-scope:release-0.293-clp-connectorfrom
anlowee:xwei/add-formatted-float

anlowee commented Sep 30, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 30, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

kirkrodrigues Oct 3, 2025

Uh oh!

kirkrodrigues left a comment

Uh oh!

kirkrodrigues left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

anlowee commented Sep 30, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Validation performed

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kirkrodrigues Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

kirkrodrigues left a comment

Choose a reason for hiding this comment

Uh oh!

kirkrodrigues left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anlowee commented Sep 30, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 30, 2025 •

edited

Loading