Re: [PR] feat(core-spec): support dataset-scoped metrics [ossie]

["christianeu-db (via GitHub)" <gi...@apache.org>]

christianeu-db commented on PR #591:
URL: https://github.com/apache/ossie/pull/591#issuecomment-6211567168

   > Two tests fail with this branch merged onto current main ([9531429](https://github.com/apache/ossie/commit/95314296b725dcff503cf2eb92e89f7cf43c5a95)), on every Python version in the matrix.
   > 
   > `validation/tests/test_ontology.py::test_split_flights_examples_match_embedded_example` compares `examples/flights.semantic_model.yaml` with the copy embedded in `examples/flights.yaml`, and the new `total_seats` metric on `AIRCRAFT` is only in the split file. `python/tests/test_serialization.py::test_the_sample_document_sets_every_declared_field` reports `datasets[0].metrics` missing from `_fully_populated_document()`, which that test asks to be extended whenever a property is added to the models. Both run in validation-ci, and the second also in python-ci.
   > 
   > The two new checks also disagree on whether names are case-sensitive. `validate_metric_scoping` casefolds, so a field `Amount` satisfies `SUM(amount)`, while the namespace check in `validate_unique_names` compares exact strings, so a field `Amount` next to a dataset-scoped metric `amount` passes as two names. A document with both validates today. Nothing else in validate.py folds case and the Metric Scoping text does not say, so whichever rule is intended, the two checks and the spec sentence should agree.
   > 
   > On the parsing, `_parse_metric_expression` is a second bare-then-`SELECT` parse of the same expression, which is the mirroring JB asked to avoid on the list. `validate_sql_expression` already performs that parse and only drops the tree. If it returned the tree, or both went through one helper that returns the tree or the error, the statement handling from #527 and any later parser fix would reach the scoping check without a second copy.
   
   @kayemkim thanks for the review
   
   1. Updated the test fixtures so CI should now be green
   2. This is a tricky case. The identifiers in expressions should be case insensitive per the expression spec. But, it is unclear if declared identifiers for fields / metrics should also be case insensitive. This a pre-existing issue so keeping with the existing validate_unique_names convention for uniqueness across names. Clearing this up broadly should be in a followup PR
   3. Applied the code extraction


-- 
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: issues-unsubscribe@ossie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org