Prajwal-banakar commented on PR #2416: URL: https://github.com/apache/fluss/pull/2416#issuecomment-3782792771
Hi @loserwang1024 and @wuchong, I was about to move the checkState validation to fromColumns as suggested, but I noticed that the strict "empty builder" rule causes failures in existing integration tests, specifically FlussAdminITCase#testAlterTableColumn. It seems some tests use a pattern where they call .column(...) to set up a template and then call .fromColumns(...) to load the remaining fields. If we enforce that the builder must be empty at the start of fromColumns, these existing tests will break. I have two thoughts on how to proceed and wanted to get your preference: Strict for IDs only: Only throw the IllegalStateException if the columns being adopted already have assigned IDs (initialization mode). This protects the schema's "source of truth" while allowing tests to append template columns. Keep it in fromSchema: Revert to keeping the check only in fromSchema(...). This ensures that when loading a full schema object, the builder is fresh, but doesn't restrict the more flexible fromColumns API. What do you think is the best approach for the project's long-term architecture? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
