Skip to content

Feat: truncate comments to engine-specific maximum#2238

Merged
treysp merged 5 commits intomainfrom
trey/comment-truncation
Mar 11, 2024
Merged

Feat: truncate comments to engine-specific maximum#2238
treysp merged 5 commits intomainfrom
trey/comment-truncation

Conversation

@treysp
Copy link
Copy Markdown
Contributor

@treysp treysp commented Mar 8, 2024

Some engines error if comments are longer than a maximum length. With this PR they will be automatically truncated to the engine's max.

Some engines, like Spark and Trino, have modular compute, metadata, and storage layers. The max comment length may differ across choices of metadata layer, such as Hive (max 4000 chars) or Iceberg (no max). For modular engines, we handle truncation in the Hive mixin rather than the engine adapter itself.

@treysp treysp requested review from crericha and eakmanrq March 8, 2024 20:51
@crericha
Copy link
Copy Markdown
Contributor

crericha commented Mar 8, 2024

Approach looks good. Nice job, @treysp. I left a couple minor comments. I didn't deeply review where the truncation was added. I'll leave that to @eakmanrq who knows that code better than I.

Copy link
Copy Markdown
Collaborator

@eakmanrq eakmanrq left a comment

Choose a reason for hiding this comment

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

Nice! One small change requested.

@treysp treysp force-pushed the trey/comment-truncation branch from 53dbc27 to 2d53972 Compare March 11, 2024 15:11
@treysp treysp force-pushed the trey/comment-truncation branch from 2d53972 to a622e22 Compare March 11, 2024 15:12
@treysp treysp merged commit 1291b07 into main Mar 11, 2024
@treysp treysp deleted the trey/comment-truncation branch March 11, 2024 15:28
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.

3 participants