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

Expected date mismatch in TDVT, "expected_setup.calcs_data3.txt", test case "time1" #665

Closed
redcatbear opened this issue Dec 9, 2020 · 10 comments · Fixed by #738
Closed

Comments

@redcatbear
Copy link
Contributor

Describe the bug
When testing the TO_TIMESTAMP function in the Exasol connector against the column "time1" in the test database, the TDVT expectations in file expected_setup.calcs_data3.txt contain a constant date of 1901-01-31.

The time1 column only contains times, no dates (note that Exasol has not pure time type).

TO_TIMESTAMP is called on the time1 column without a format string, leading to an exception instead of the expected values. If I add a format string that parses time only, the default date will be set to 0001-01-01. So the expected values in the TDVT test dataset cannot be matched.

We are trying to figure out where exactly that expected date comes from 1901-01-31 and why it is not 0001-01-01. We are also puzzled how that test could ever have been green, given that the format string is missing and the default date is a mismatch.

Screenshots
image

Desktop (please complete the following information):

  • OS: Windows 10 Pro 20H2
  • Tableau Version: 2020.3.1
  • Connector SDK:
    commit d433fe6963d417dcbc49c379955c88f75cfec8c2 (HEAD -> master, origin/master, origin/HEAD)
    Merge: cc43c30 2f9dc15
    Author: Jainam Shah <redacted>
    Date:   Mon Oct 19 10:36:09 2020 -0700
    

About you:
Name: redcatbear
Company: Exasol

@redcatbear
Copy link
Contributor Author

As a side-note: having expected values that are product-specific in the central test library does not seem ideal to me. Connectors and external products have a separate lifecycle from tableau, so these test cases should be in the connector sources, not in TDVT.

@redcatbear
Copy link
Contributor Author

I just realized #169 is closely related.
And I manipulated the connector to produce 1901-01-31 in case of missing date?
Other dates produced %error% in Tableau.
This smells a lot like a magic number. Is that correct?

@pvanderknyff
Copy link
Contributor

According to the comments on that issue, 190-01-31 is an internal default date for Tableau when a date must be attached. However, it's not required, TDVT does accept times without the date attached since we have multiple expected files.

However, I don't think that's your issue - the sql query generated by Tableau is returning null for this test, which is why the test is failing. The calcs_data test is a simple select statement on a column designed to ensure your data is loaded correctly. Double check that you can see the times in this column when connecting to your database, both from Tableau and a non-Tableau application.

As an aside, today TDVT will only show one set of expected tuples, and unfortunately it picks from the first expected file and not the closest matching expected file. The column "Closest Expected" in the TDVT results will tell you the number of the expected file it most closely matches, though it's not on the TDVT dashboards by default. Our team has an item on our backlog to clean up the messaging around this.

Let me know if you have any questions.

@redcatbear
Copy link
Contributor Author

@pvanderknyff, thanks for the clarification. It is good to know that 1901-01-31 is in fact a magic number. Exasol has no pure time type, so not providing a date in the cast to timestamp is unfortunately not an option.
I changed the connector code, so that the cast recognizes columns that contain only a time represented as VARCHAR and in that case add that magic date to the timestamp. This also turned the test green.

@redcatbear
Copy link
Contributor Author

As far as I understand your answer about the "closest match", that confirms that TDVT really iterates over all expectation files and considers a test case successful if at least one matches?

That also means that Oracle or Teradata expectations are tested against an Exasol and vice-versa?

@pvanderknyff
Copy link
Contributor

Yes, TDVT checks the expected files until it finds a match. If the test matches any one of the expected files, it is considered passed. The only reason some of the expected have database names in them is because we originally generated them using our internal connectors, you don't have to match a specific expected file to pass TDVT.

I'm also going to walk back what I said, and what was said in the previous issue, about the 1901-01-31 being a magic number (I was thinking of 1900-01-01). I double checked with my team, and it's not in our code at all. What happened is that we run TDVT internally for our connectors against test instances, and our exasol instance had data loaded in its time1 column that was inconsistent with most other connectors. For some reason had the 1901-01-31 date added. This is why that expected file exists and is looking for 1901-01-31. Adding a story to my team's backlog to look at fixing that and if possible remove this confusing expected file.

Since you're writing a new connector, you should be matching the other expected files and shouldn't be adding 1901-01-31 artificially. The other expected files agree that time1 should look like the following (no date prefix):
#00:05:57#
#2:05:25#
#4:40:49#
#4:48:07#
#9:33:31#
etc.

Apologies for the confusion.

@yellowYou yellowYou added the investigate Issue the team needs to look at label Dec 23, 2020
@pvanderknyff
Copy link
Contributor

@redcatbear Have you had any luck getting the test to pass?

@redcatbear
Copy link
Contributor Author

redcatbear commented Jan 11, 2021

Hi @pvanderknyff.

Thanks for the clarifications. Especially for explaining how 1901-01-31 came into existence and that it is not indeed a magic number.
As I mentioned before, Exasol does not feature a "time-only" type. If you want to note down times, you either have to use a VARCHAR or use a timestamp, accepting that it also contains a date. The default date is 0001-01-01. I think you should change the test case to this effect, so that I can remove my extra code that adds the non-standard date.
So in my eyes the best cause of action would be to change all instances of 1901-01-31 to 0001-01-01 and keep the file.

Would that be possible?

@pvanderknyff
Copy link
Contributor

Hi Sebastian,

Internally, Tableau also doesn't have a time type, we use datetime in those situations. I think the dialect we provided for you of our current Exasol connector's dialect has the casts for time use the 1900-01-01 epoch, and even though your default date is different the dialect should work since it's consistent across connectors in Tableau.

Do you have any other date or time related test failures? If not, you can safely skip this test (the calcs_data one). It doesn't test anything but that the data is loaded, and that information is only useful in triaging other test failures. While we could add a new expected file to match this particular case, we may end up in another situation where someone down the road is wondering why this particular magic number was chosen, and from talking with my team and doing some other investigating I think this may be some subtly in how the data was loaded, not a problem with the connector.

Thanks for your patience.

@redcatbear
Copy link
Contributor Author

Understood. I will disable the test so that you can remove the expectation file.
This ticket can then be closed.

Thanks for the support, also to the team. I appreciate it. This is how an Open Source Community should work.

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 a pull request may close this issue.

4 participants