Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override function TIME1899 #32

Merged
merged 1 commit into from
Sep 1, 2023
Merged

Override function TIME1899 #32

merged 1 commit into from
Sep 1, 2023

Conversation

hab6
Copy link
Collaborator

@hab6 hab6 commented Sep 1, 2023

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
Copy link
Member

clach04 commented Sep 1, 2023

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 #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 clach04 merged commit 54a865d into master Sep 1, 2023
@clach04
Copy link
Member

clach04 commented Sep 1, 2023

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 #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.

@hab6 hab6 deleted the hab6-translate-time-type branch September 18, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants