dengc367 commented on PR #14332: URL: https://github.com/apache/gravitino/pull/14332#issuecomment-6490079370
I implemented a working planner against this SPI to validate it end to end (a Spark SQL compiler, ~2.3k lines with 43 tests, branch [dengc367:feat/semantic-query-planner](https://github.com/dengc367/gravitino/tree/feat/semantic-query-planner)). The SPI is a good shape and the endpoint/error-code work is solid. These are the eight places where writing a real planner ran into the contract. I split them into what I would fix in this PR (cheap now, breaking later) and what is fine to defer. ## Worth fixing in this PR **1. Filters cannot express the common predicates.** `SemanticQueryFilter.Operator` is `EQ, NE, GT, GE, LT, LE`. There is no `IN`, `IS NULL` / `IS NOT NULL`, `LIKE`, or `BETWEEN`, so "revenue in the EMEA and APAC regions" or "customers with no segment" cannot be asked at all, and a planner has no way to signal it other than throwing. Since the enum is closed, adding cases later is source-incompatible for every implemented planner. Suggest at minimum `IN` (with a value list), `IS_NULL` / `IS_NOT_NULL`, and `LIKE`. **2. `Metric` does not model aggregation.** `Metric` carries only a name and an expression, so the planner has to guess whether an expression aggregates in order to decide which selections go into `GROUP BY`. My implementation detects `SUM(`/`COUNT(`/`AVG(`/… textually, which is exactly the kind of string parsing a typed SPI should remove — a metric written as `total / count` or with a quoted `"SUM(x)"` literal breaks it. Suggest either an `Aggregation` enum on `Metric` (`SUM`, `COUNT`, `COUNT_DISTINCT`, `AVG`, `MIN`, `MAX`, `NONE`) or a documented invariant that a metric expression is always an aggregate over one dataset. **3. `Relationship` carries no cardinality.** Every hop has to be an `INNER JOIN`, because a `LEFT JOIN` would silently change aggregate semantics and the API gives no basis for the choice. For a semantic layer this is load-bearing: "orders LEFT JOIN customers" and "orders INNER JOIN customers" produce different revenue. Suggest `cardinality` (`ONE_TO_ONE`, `MANY_TO_ONE`, `ONE_TO_MANY`, `MANY_TO_MANY`) or at least an `optional`/`nullable` flag on the relationship. **4. `SemanticQueryPlan` has no provenance.** The plan returns SQL, dialect and planner name, but not which datasets and relationships were used, so a caller cannot check that the plan read what they expected and an audit log cannot record it. My workaround is a separate `resolvedIdentifiers(model, query)` helper on the planner, which every planner will have to reinvent. Suggest a `sources` field (dataset identifiers, or `NameIdentifier`s with aliases) on the plan. **5. `SemanticQuery` has no limit.** The only place to put one is a planner-specific `PlanningContext` property, which makes it invisible in the request contract and different per planner. `LIMIT` is a query-level concept; suggest a `@Nullable Integer limit` on `SemanticQuery`. **6. `dialect` is a free-form string.** Two planners will accept two different sets of spellings (`spark`, `spark-sql`, `spark_sql`, `databricks`), and a caller cannot know which. Either an enum, or a documented canonical value plus a `supportedDialects()` accessor on the planner. In my planner `dialect` lives on `SemanticQuery` while `PlanningContext` has no dialect, which also reads oddly — a plan is a (query, dialect, model) product. ## Fine to defer, but worth documenting **7. Metrics are top-level, not dataset-scoped,** so the owning dataset is inferred from the `<dataset>.<column>` qualifiers in the expression, falling back to the declared fields it mentions. That works, but it is an unwritten convention: please put it in the `Metric` Javadoc so all planners infer ownership the same way, and say that an ambiguous metric must be qualified. **8. Discovery is config plus reflection,** with no provider interface or `ServiceLoader` hook. That is a legitimate pattern and I did not invent a provider, but it is only discoverable by reading `SemanticQueryPlannerFactory`. A sentence in the `SemanticQueryPlanner` Javadoc with the two config keys and "put the jar on the server classpath" would save every implementor the search. Nothing here blocks the endpoint itself, and items 1–4 are the ones that would bite a second planner implementation. Happy to adjust my branch if any of these land. (Also: the branch is still `CONFLICTING`; the rebase is in the comment above.) -- 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]
