zeroshade commented on code in PR #2318:
URL: https://github.com/apache/iceberg-go/pull/2318#discussion_r4379785020
##########
udf/metadata.go:
##########
@@ -91,17 +91,17 @@ type SQLRepresentation struct {
SQL string `json:"sql"`
}
-// NewSQLRepresentation creates a SQL representation from a dialect
-// identifier (e.g. "spark", "trino") and a SQL expression.
-func NewSQLRepresentation(dialect, sql string) (SQLRepresentation, error) {
- if dialect == "" {
+// NewSQLRepresentation validates a SQL representation.
+// Named fields prevent mixing up the dialect and SQL, which are both strings.
+func NewSQLRepresentation(repr SQLRepresentation) (SQLRepresentation, error) {
+ if repr.Dialect == "" {
return SQLRepresentation{}, fmt.Errorf("%w: sql representation
requires a dialect", ErrInvalidUDFMetadata)
}
- if sql == "" {
+ if repr.SQL == "" {
return SQLRepresentation{}, fmt.Errorf("%w: sql representation
requires a sql expression", ErrInvalidUDFMetadata)
}
- return SQLRepresentation{Dialect: dialect, SQL: sql}, nil
+ return repr, nil
Review Comment:
Returning the argument unchanged is the giveaway: this is validation, not
construction. If an early check is worth keeping, `func (r SQLRepresentation)
Validate() error` expresses that without the pass-a-struct-get-it-back shape.
##########
udf/metadata.go:
##########
@@ -91,17 +91,17 @@ type SQLRepresentation struct {
SQL string `json:"sql"`
}
-// NewSQLRepresentation creates a SQL representation from a dialect
-// identifier (e.g. "spark", "trino") and a SQL expression.
-func NewSQLRepresentation(dialect, sql string) (SQLRepresentation, error) {
- if dialect == "" {
+// NewSQLRepresentation validates a SQL representation.
+// Named fields prevent mixing up the dialect and SQL, which are both strings.
+func NewSQLRepresentation(repr SQLRepresentation) (SQLRepresentation, error) {
Review Comment:
`SQLRepresentation`'s fields are exported, so
`udf.SQLRepresentation{Dialect: "trino", SQL: "x + 1"}` already names both
strings and can't be swapped. This function now adds only the empty-field
checks, and the UDF metadata validation already does those (`udf/metadata.go`,
around line 356) and also rejects duplicate dialects, which this can't. Could
we drop `NewSQLRepresentation` and have callers use the struct literal?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
