You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ossie.apache.org by "RyutoYoda (via GitHub)" <gi...@apache.org> on 2026/10/16 05:19:56 UTC
[PR] fix(salesforce): report calculated fields omitted for their dialect [ossie]
RyutoYoda opened a new pull request, #594:
URL: https://github.com/apache/ossie/pull/594
## Summary
Export has no translation for a calculation written in any dialect but Tableau,
so it omits the field until a common calculation dialect is agreed. It did so
silently, and a model imported from a warehouse carries its calculations in that
warehouse's dialect, so the exported model is quietly short of fields.
Name the field and its dialect in a warning, in the same shape as the warnings
the datatype mapping already emits.
## Verification
- `mvn -B clean verify` in `converters/salesforce`: 352 tests, 0 failures, 0 errors, `apache-rat:check` clean.
- Output is unchanged: a model with a `SNOWFLAKE` calculated field converts
byte-identically before and after, with the warning alongside.
- A new test covers the warning naming the field and the dialect.
## Related Issues
No existing issue covers this.
## Checklist
### Converters
- [x] Converter changes include tests
### Documentation
- [x] `converters/salesforce/README.md` states that the field is omitted with a warning
### Tests
- [x] All tests pass locally
- [x] New test added
### Compliance
- [x] No third-party code added
- [x] Apache RAT check passes
--
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
Re: [PR] fix(salesforce): report calculated fields omitted for their dialect [ossie]
Posted by "RyutoYoda (via GitHub)" <gi...@apache.org>.
RyutoYoda commented on code in PR #594:
URL: https://github.com/apache/ossie/pull/594#discussion_r4293427409
##########
converters/salesforce/src/main/java/org/apache/ossie/converter/FieldMappingHandler.java:
##########
@@ -356,6 +356,11 @@ private void processFieldsForDataset(
// Skip calculated fields for non-Tableau dialects till we agree on a common dialect.
if (!DIALECT_TABLEAU.equals(dialect) && isCalculatedExpression(expression, dialect)) {
+ logger.warn(
Review Comment:
Added. The warning now reads: `Field 'CITY_UPPER' in dataset 'CUSTOMERS' has a calculated 'SNOWFLAKE' expression, which export only translates in the 'TABLEAU' dialect; omitting the field`.
##########
converters/salesforce/src/main/java/org/apache/ossie/converter/FieldMappingHandler.java:
##########
@@ -356,6 +356,11 @@ private void processFieldsForDataset(
// Skip calculated fields for non-Tableau dialects till we agree on a common dialect.
if (!DIALECT_TABLEAU.equals(dialect) && isCalculatedExpression(expression, dialect)) {
Review Comment:
Yes, but not through `convert()`: the Ossie schema makes `dialect` required, so a null one only arrives when the handler is driven directly. That case now reports the expression as declaring no dialect instead of as the dialect `null`, with a test covering it.
On omissions only being logged: agreed, but reporting them to the caller means adding a return value to the handler API, so I left that out of this PR.
--
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
Re: [PR] fix(salesforce): report calculated fields omitted for their dialect [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db commented on code in PR #594:
URL: https://github.com/apache/ossie/pull/594#discussion_r4291976857
##########
converters/salesforce/README.md:
##########
@@ -277,6 +277,8 @@ and a measurement as `Number`.
For direct fields, the presence of the `dimension` object determines whether the
field is exported to `semanticDimensions` or `semanticMeasurements`. A calculated
Tableau expression follows the converter's existing calculated-dimension path.
+A calculation written in any other dialect has no translation until a common
+calculation dialect is agreed, so the field is omitted with a warning naming it.
Review Comment:
FYI: OSSIE_SQL_2026 under core-spec/expression_language.md should be the portable dialect
--
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
Re: [PR] fix(salesforce): report calculated fields omitted for their dialect [ossie]
Posted by "RyutoYoda (via GitHub)" <gi...@apache.org>.
RyutoYoda commented on code in PR #594:
URL: https://github.com/apache/ossie/pull/594#discussion_r4293432431
##########
converters/salesforce/src/test/java/org/apache/ossie/OssieToSalesforceConverterTest.java:
##########
@@ -233,6 +233,31 @@ void testCalculatedFieldDetection() throws Exception {
assertNull(ansiCalcDimensions, "ANSI_SQL dialect: no semanticCalculatedDimensions");
}
+ @Test
+ void testCalculatedFieldDroppedForItsDialectIsReported() throws Exception {
+ // Export has no translation for a calculation written in any dialect but Tableau,
+ // so it omits the field. Omitting it without a word leaves the exported model
+ // quietly short of fields the Ossie document declares.
+ Logger fieldLogger = (Logger) LoggerFactory.getLogger(FieldMappingHandler.class);
+ ListAppender<ILoggingEvent> warnings = new ListAppender<>();
+ warnings.start();
+ fieldLogger.addAppender(warnings);
+ try {
+ converter.convert(ossieYamlAnsiSql);
+ } finally {
+ fieldLogger.detachAppender(warnings);
+ }
Review Comment:
Done, all three. The test now asserts the message names the field, its dataset and its dialect together, that the field is absent from the exported payload, and that a Tableau calculated field is exported with no warning.
##########
converters/salesforce/README.md:
##########
@@ -277,6 +277,8 @@ and a measurement as `Number`.
For direct fields, the presence of the `dimension` object determines whether the
field is exported to `semanticDimensions` or `semanticMeasurements`. A calculated
Tableau expression follows the converter's existing calculated-dimension path.
+A calculation written in any other dialect has no translation until a common
+calculation dialect is agreed, so the field is omitted with a warning naming it.
Review Comment:
Thanks, corrected. The README no longer ties this to a dialect still being agreed, and neither does the comment above the check.
--
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
Re: [PR] fix(salesforce): report calculated fields omitted for their dialect [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #594:
URL: https://github.com/apache/ossie/pull/594#discussion_r4292207056
##########
converters/salesforce/src/main/java/org/apache/ossie/converter/FieldMappingHandler.java:
##########
@@ -356,6 +356,11 @@ private void processFieldsForDataset(
// Skip calculated fields for non-Tableau dialects till we agree on a common dialect.
if (!DIALECT_TABLEAU.equals(dialect) && isCalculatedExpression(expression, dialect)) {
+ logger.warn(
Review Comment:
Nit: this warning names the field but not its dataset. Different datasets can share a field name like `order_year`, so it's hard to tell which one lost the field.
`ossieDataset` is already in scope here, so could we include its name in the warning?
##########
converters/salesforce/src/main/java/org/apache/ossie/converter/FieldMappingHandler.java:
##########
@@ -356,6 +356,11 @@ private void processFieldsForDataset(
// Skip calculated fields for non-Tableau dialects till we agree on a common dialect.
if (!DIALECT_TABLEAU.equals(dialect) && isCalculatedExpression(expression, dialect)) {
Review Comment:
Question: can `unwrapExpression` return a null dialect when none is declared? If so, `!DIALECT_TABLEAU.equals(dialect)` also matches. The field is dropped and the log reads `calculated 'null' expression`. I believe it is possible, it would be worth a clearer message for a missing dialect. Since omissions are only logged, a caller that doesn't capture logs would not know fields were dropped.
##########
converters/salesforce/src/test/java/org/apache/ossie/OssieToSalesforceConverterTest.java:
##########
@@ -233,6 +233,31 @@ void testCalculatedFieldDetection() throws Exception {
assertNull(ansiCalcDimensions, "ANSI_SQL dialect: no semanticCalculatedDimensions");
}
+ @Test
+ void testCalculatedFieldDroppedForItsDialectIsReported() throws Exception {
+ // Export has no translation for a calculation written in any dialect but Tableau,
+ // so it omits the field. Omitting it without a word leaves the exported model
+ // quietly short of fields the Ossie document declares.
+ Logger fieldLogger = (Logger) LoggerFactory.getLogger(FieldMappingHandler.class);
+ ListAppender<ILoggingEvent> warnings = new ListAppender<>();
+ warnings.start();
+ fieldLogger.addAppender(warnings);
+ try {
+ converter.convert(ossieYamlAnsiSql);
+ } finally {
+ fieldLogger.detachAppender(warnings);
+ }
Review Comment:
I have a suggestion.
This only checks that `order_year` appears in a WARN message. A regression could log the wrong dialect, or warn for Tableau calculated fields that are still exported, and this test would still pass.
Maybe we could assert that:
- the message names the dialect
- the field is absent from the exported payload
- a Tableau calculated field exports with no warning?
Matching the whole message, or at least the field and dataset together, would also avoid a false match on an unrelated warning.
--
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
Re: [PR] fix(salesforce): report calculated fields omitted for their dialect [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre merged PR #594:
URL: https://github.com/apache/ossie/pull/594
--
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