Skip to content

feat: Add clp_get_json_string UDF.#82

Merged
wraymo merged 4 commits into
y-scope:release-0.293-clp-connectorfrom
wraymo:clp_get_json_string
Oct 27, 2025
Merged

feat: Add clp_get_json_string UDF.#82
wraymo merged 4 commits into
y-scope:release-0.293-clp-connectorfrom
wraymo:clp_get_json_string

Conversation

@wraymo

@wraymo wraymo commented Oct 18, 2025

Copy link
Copy Markdown

Description

This PR introduces the clp_get_json_string UDF, which is rewritten into a special column named __json_string during UDF rewriting. With Velox support, this UDF enables fetching a serialized JSON string representation of a row.

Note: clp_get_json_string is currently supported only in projection fields.

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

Did an end-to-end testing with a sample dataset

presto:default> select timestamp, clp_get_json_string() from default where clp_wildcard_string_column() in ('profile_update', 'logout');
      timestamp       |                                                                          _col1                                                                           
----------------------+----------------------------------------------------------------------------------------------------------------------------------------------------------
 2025-05-13T15:00:20Z | {"timestamp":"2025-05-13T15:00:20Z","level":"INFO","service":"user-service","event":{"action":"logout"},"user":{"id":"12345"}}                           
 2025-05-13T15:00:06Z | {"timestamp":"2025-05-13T15:00:06Z","level":"INFO","service":"user-service","event":{"action":"profile_update"},"user":{"id":"67890","name":"Jane Doe"}} 
 2025-05-13T15:00:20Z | {"timestamp":"2025-05-13T15:00:20Z","level":"INFO","service":"user-service","event":{"action":"logout"},"user":{"id":"12345"}}                           
 2025-05-13T15:00:06Z | {"timestamp":"2025-05-13T15:00:06Z","level":"INFO","service":"user-service","event":{"action":"profile_update"},"user":{"id":"67890","name":"Jane Doe"}} 
(4 rows)

Query 20251018_182705_00001_ikv76, FINISHED, 1 node
Splits: 3 total, 3 done (100.00%)
[Latency: client-side: 225ms, server-side: 191ms] [6 rows, 0B] [31 rows/s, 0B/s]

presto:default> select event.type, clp_get_json_string() from default_ir;
      type      |                                                                                                                            _col1                                                                                     >
----------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------->
 network        | {"event":{"details":{"destination":{"ip":"10.0.1.5","port":80},"source":{"ip":"192.168.3.10"}},"severity":"INFO","subtype":"connection","tags":["inbound","tcp"],"type":"network"},"timestamp":1746003000}           >
 storage        | {"event":{"details":{"mount":"/var/log","usage":{"percent":92}},"severity":"WARNING","subtype":"disk_usage","tags":["filesystem","monitoring"],"type":"storage"},"timestamp":1746003005}                             >
 process        | {"event":{"details":{"id":1234,"name":"cron"},"severity":"INFO","subtype":"start","tags":["system","init"],"type":"process"},"timestamp":1746003010}                                                                 >
 authentication | {"event":{"details":{"ip":"203.0.113.20","reason":"invalid password"},"severity":"ERROR","subtype":"failure","tags":["login","security"],"type":"authentication","user":{"method":"ssh","name":"guest"}},"timestamp":>
 service        | {"event":{"details":{"name":"httpd","state":"running","uptime":{"seconds":3600}},"severity":"INFO","subtype":"status","tags":["daemon","http"],"type":"service"},"timestamp":1746003020}                             >
 security       | {"event":{"details":{"payload":"SELECT * FROM users;","rule":{"id":"SQLI-001"},"target":"/api/data"},"severity":"HIGH","subtype":"alert","tags":["threat","sql"],"type":"security"},"timestamp":1746003025}          >
 memory         | {"event":{"details":{"free":{"gb":7.5},"total":{"gb":16},"used":{"gb":8.5}},"severity":"NORMAL","subtype":"usage","tags":["resources"],"type":"memory"},"timestamp":1746003030}                                      >
 configuration  | {"event":{"details":{"component":"database","setting":{"name":"timeout","newValue":60,"oldValue":30}},"severity":"INFO","subtype":"change","tags":["admin","settings"],"type":"configuration","user":{"name":"admin"}>
 file           | {"event":{"details":{"flags":["sensitive","audit"],"operation":"read","path":"/etc/shadow","permissions":"rw-------"},"severity":"WARNING","subtype":"access","tags":["file","privileged"],"type":"file","user":{"nam>
 backup         | {"event":{"details":{"duration":{"seconds":1200},"job":{"name":"daily_backup","status":"completed"},"size":{"gb":25.3}},"severity":"INFO","subtype":"status","tags":["backup","daily"],"type":"backup"},"timestamp":1>
(10 rows)

Query 20251022_215732_00010_fizjr, FINISHED, 1 node
Splits: 2 total, 2 done (100.00%)
[Latency: client-side: 137ms, server-side: 111ms] [10 rows, 0B] [90 rows/s, 0B/s]

Summary by CodeRabbit

  • New Features

    • Added a scalar function to extract JSON string data from CLP-compressed columns in Presto queries.
  • Improvements

    • Improved query rewrite and optimization logic for CLP functions, including better handling of projections and push-downs to improve performance of JSON string retrieval.
  • Tests

    • Updated and added tests covering JSON string handling and CLP query optimization scenarios.

@coderabbitai

coderabbitai Bot commented Oct 18, 2025

Copy link
Copy Markdown

Walkthrough

Adds a new scalar function CLP_GET_JSON_STRING to the Presto CLP plugin, updates the CLP UDF rewriter to handle this function specially within ProjectNode contexts by emitting a placeholder variable and synthetic column mapping, and updates/renames tests; also updates a Velox submodule commit hash.

Changes

Cohort / File(s) Summary
New Function Definition
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFunctions.java
Added public scalar function clpGetJSONString() annotated as a @ScalarFunction returning VARCHAR (Slice). Implementation currently throws UnsupportedOperationException.
UDF Rewriting Logic
presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpUdfRewriter.java
Added JSON_STRING_PLACEHOLDER constant. Extended rewriteClpUdfs() to accept inProjectNode boolean and propagate it. Implemented special-case rewriting for CLP_GET_JSON_STRING when inProjectNode == true: generate a placeholder variable, record a synthetic ClpColumnHandle→variable mapping, and integrate the variable into TableScanNode outputs and project assignments. Recursive rewrites and predicate rewrites propagate inProjectNode appropriately.
Tests
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpUdfRewriter.java
Renamed tests: testScanFiltertestClpGetScanFilter, testScanProjecttestClpGetScanProject, testScanProjectFiltertestClpGetScanProjectFilter. Added testClpGetJsonString() to verify rewriting of CLP_GET_JSON_STRING() in projection contexts including expected placeholder projection and CLP table scan pattern.
Submodule Update
presto-native-execution/velox
Updated Velox submodule commit reference from cf895960f83e... to 425b66f6cb39...; no functional code changes in this diff.

Sequence Diagram

sequenceDiagram
    participant Q as QueryPlanner
    participant R as ClpUdfRewriter
    participant P as ProjectNode
    participant T as TableScanNode

    Q->>R: rewriteClpUdfs(planRoot, inProjectNode=false)
    alt encountering ProjectNode
        R->>P: inspect assignments
        Note right of R #DDEBF7: set inProjectNode = true
        R->>R: rewrite expressions in project (inProjectNode=true)
        alt sees CLP_GET_JSON_STRING()
            R->>R: create placeholder var (JSON_STRING_PLACEHOLDER)
            R->>T: add synthetic ClpColumnHandle -> placeholder var mapping
            R->>P: replace expression with placeholder var
        end
        R-->>Q: return modified ProjectNode + updated TableScan outputs
    else encountering FilterNode
        Note right of R #F7E7E7: inProjectNode = false
        R->>R: standard CLP_GET_* rewriting in predicate
        R-->>Q: return modified predicate
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay attention to: rewriteClpUdfs signature change and all call sites; correctness of inProjectNode propagation.
  • Verify integration of placeholder variable into TableScanNode outputs and assignments.
  • Review new test testClpGetJsonString() for expected patterns and stability.
  • Check serialization/equality of synthetic ClpColumnHandle used as map key.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description covers the main content areas (description of the UDF, motivation with Velox support reference, impact regarding projection-field-only support, and validation with end-to-end testing examples), but it deviates from the required template structure in critical ways. Most importantly, the description is missing the Release Notes section entirely—the template explicitly requires either properly filled-in release notes or the "== NO RELEASE NOTE ==" marker, neither of which appears in the submission. Additionally, the description uses a custom checklist format rather than the specific contributor checklist outlined in the template, and the sections are organized differently than the template specifies. Please update the pull request description to include the Release Notes section as required by the template. Either provide detailed release notes following the guidelines or explicitly state "== NO RELEASE NOTE ==" if no release notes are needed. Additionally, reorganize the description to match the template's structure more closely, including the explicit "Motivation and Context," "Impact," and "Test Plan" sections, and use the template's standard contributor checklist format.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "feat: Add clp_get_json_string UDF." directly and accurately describes the primary change in the changeset. The PR introduces a new UDF function clp_get_json_string across multiple files, including the function definition in ClpFunctions.java, rewriting logic in ClpUdfRewriter.java, and corresponding test coverage in TestClpUdfRewriter.java. The title is concise, uses clear and specific language, employs the conventional "feat:" prefix to indicate a feature addition, and avoids vague terminology. A teammate scanning the PR history would immediately understand that this commit adds a new UDF capability to the system.
✨ 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.

@wraymo
wraymo marked this pull request as ready for review October 18, 2025 18:32
@wraymo
wraymo requested a review from gibber9809 October 27, 2025 14:10

@gibber9809 gibber9809 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really just had one comment, but otherwise looks good.

expression.getSourceLocation(),
JSON_STRING_PLACEHOLDER,
call.getType());
ClpColumnHandle targetHandle = new ClpColumnHandle(JSON_STRING_PLACEHOLDER, call.getType());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to record in ClpColumnHandle that this is a JSON string like we talked about in the other PR, or do we plan on leaving that for future work?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I linked an issue with this PR. We can probably leave it to the future work. One possible fix is to add a field isJsonStringColumn in ClpColumnHandle.

@gibber9809
gibber9809 self-requested a review October 27, 2025 19:59

@gibber9809 gibber9809 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. For the PR title, I think we just need to add a period:

feat: Add `clp_get_json_string` UDF.

@wraymo wraymo changed the title feat: Add clp_get_json_string UDF feat: Add clp_get_json_string UDF. Oct 27, 2025
@wraymo
wraymo merged commit d02268d into y-scope:release-0.293-clp-connector Oct 27, 2025
58 of 59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants