search

ActianCorp / actian_tableau_connector

Tableau connector (aka taco) for Actian Avalanche, Vector, and Ingres
https://extensiongallery.tableau.com/products/936
Apache License 2.0
1 stars 4 forks source link

Override function TIME1899 #32

Closed hab6 closed 1 year ago

hab6 commented 1 year ago

Symptom

The TDVT test case calcs_data.time/time1 fails to match expected results as documented in Jira ticket II-12232. A similar failure (with same root cause) occurs when performing an Extract Data operation as required by Tableau Manual QA Tests.

Cause

The DDL of the Calcs table in the Tableau TDVT dataset defines the time1 column as type TIME and yet Tableau itself does not have a TIME data type (references A, B, C, D), thus workarounds are required to extract and/or display only the time portion of DATETIME/TIMESTAMP data in Tableau.

Tableau Built-in Function

The Tableau Connector SDK sample dialect SQLDialect contains a function called TIME1899 that overrides a Tableau built-in function of the same name, which translates time values so that they contain a pre-pended date of 1899-12-30. In other words, the purpose of the TIME1899 function is to translate data source TIME data into DATETIME data that Tableau can handle which later can be dealt with in Tableau for extracting parts of the datetime value as needed.

Proposed Solution

Override the built-in TIME1899 function so that "time" data returned by the JDBC and ODBC drivers is preserved by the Actian connector(s). The Tableau built-in TIME1899 function still fixes up the "time" data (somewhere before the data is shown in Tableau Desktop) with the fixed date of 1899-12-30, but the dialect fix allows TDVT and manual QA tests to succeed.

Test Results

The proposed solution works both with the TDVT suite and the "Extract Data" operation of the Manual QA tests. The fix allows the TDVT test calcs_data.time/time1 to succeed in matching expected results and no adverse affects are seen with any other TDVT test cases.

clach04 commented 1 year ago

I'm going to approve for expediency (and because you are saying this increases the pass count) BUT I do have feedback and would like to see some changes made (post merge). Opened https://github.com/ActianCorp/actian_tableau_connector/issues/33

Requested Change 1

The explanation you have in the merge description is worth placing in code comments (i.e. XML comments), including the link to https://github.com/tableau/connector-plugin-sdk/blob/4cbc26c6c358224774cb8e5371f6b5292dbe531c/samples/components/dialects/SQLDialect.tdd#L51C38-L51C38

Requested Change 2

I want to see more information/explanation about why our implementation is returning time rather than datetime and why it is not using the magic date of 1899-12-30. My reading was that Tableau doesn't have a proper time datatype based on the explanation, I'm wondering if perhaps this is a workaround Tableau has for DBMS that don't have time only?

clach04 commented 1 year ago

I made a mistake; the PR explanation text did NOT get put into the commit message (we'll know for next time, needs to go into to original commit messae). Which increases the need to implement https://github.com/ActianCorp/actian_tableau_connector/issues/33 - I understand that this may need to fall behind the other "fix it" issues that are needed to get pass rate to acceptable level.