-
Notifications
You must be signed in to change notification settings - Fork 1.8k
refactor(backend): introduce dialect-aware LargeText and replace longtext (+call-site type adjustments). Part of #12063 #12163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hi @kaikaila. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
🚫 This command cannot be processed. Only organization members or owners can use the commands. |
/ok-to-test |
@kaikaila is this PR still WIP? |
Hi @mprahl , thanks for checking in! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
It's unfortunate that we have to change Go types but it seems there's no other way. Thanks for digging into this!
@kaikaila it looks like there are some merge conflicts. Let me know when they are resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
Thanks a lot @mprahl ! I really appreciate your review and the LGTM. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mprahl The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: kaikaila <[email protected]>
/lgtm |
…text (+call-site type adjustments). Part of kubeflow#12063 (kubeflow#12163) Signed-off-by: kaikaila <[email protected]>
Introduce an internal LargeText model type and replace all
gorm: type:longtext
usages. LargeText implementsGormDBDataType()
to map to MySQL=LONGTEXT and Postgres=TEXT. This unblocks Postgres AutoMigrate while keeping MySQL schemas unchanged.Motivation
Postgres fails on migrate with type "longtext" does not exist (SQLSTATE 42704). Making the type dialect-aware removes this blocker.
Scope
Although this PR is “models-first”, some call sites needed type conversions/wiring only so code compiles and tests run. No query semantics or business logic were changed.
Backward compatibility
Testing
Notes for reviewers
Changelog
Internal refactor; no user-facing change. No release note needed.
Part of #12063.
Reference
This PR is part of RFC discussion in #12047.
Technical Appendix
Click to expand full investigation summary
🔍 Background & Rationale
Kubeflow Pipelines previously relied on
gorm:"size:65535"
to represent large string fields (e.g., descriptions, manifests, parameters).In GORM v1, this produced compatible behavior:
LONGTEXT
TEXT
However, in GORM v2, internal dialect logic has changed:
size:65535
asMEDIUMTEXT
, which may be too small for our payloadsTEXT
, but will fail iftype:longtext
is explicitly used (since Postgres doesn’t recognize it)VARCHAR(65535)
in MySQL can exceed row size limits and lead to errorsThus, the old approach is no longer reliable for cross-database compatibility.
⸻
✅ Chosen Solution: Define a LargeText Custom Type
Following GORM official recommendation, We introduce a dialect-aware LargeText type:
This approach is :
• Ensures correct behavior for both MySQL and PostgreSQL
• Avoids manual ALTER COLUMN migrations
• Makes the semantic intent (this is a large string field) explicit in code
⸻
🛠 Refactor Scope
We’ve audited existing fields and begun migrating high-risk ones like:
• Description
• Parameters
• PipelineSpecURI
This surfaced ~13 compile-time errors due to type mismatches, primarily in:
• Struct literal initializations
• Function parameter type checks
Most of these are resolved with explicit conversions like LargeText(x) or string(x).
⸻
🚨 Fallback Plan
If this refactor proves too invasive, we will fall back to a simpler (but less elegant) approach, which currenly is adopted in the GORM-v1 kfp:
• Keep gorm:"type:longtext" for MySQL only fields
• Use db.Migrator().AlterColumn(...) during DB init to patch schema post-facto