fix(dataset): retain is_dttm if set on metadata sync

[villebro]

SUMMARY

When syncing dataset metadata, the is_dttm flag is reset for columns that are not natively temporal (like VARCHAR that has a datetime format). This changes the logic so that is_dttm is only reset if it is unset and the new datatype would be natively identified as being temporal.

Closes #16774

AFTER

metadata-after.mp4

BEFORE

metadata-before.mp4

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[ktmud]

LGTM. Thanks for the quick fix!

For the case when the original temporal flag was inferred from db type instead of user selection, this change seems will also retain the flag, but maybe that's OK?

[villebro]

LGTM. Thanks for the quick fix!

For the case when the original temporal flag was inferred from db type instead of user selection, this change seems will also retain the flag, but maybe that's OK?

Yeah, I didn't think that would be a common need, but I'm open to changing that, too, if this is a typical scenario. Do you think it's common to want to disable temporal flags for temporal type columns?

[codecov]

Codecov Report

Merging #16776 (43e33b5) into master (a743526) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 43e33b5 differs from pull request most recent head a6cc029. Consider uploading reports for the commit a6cc029 to get more accurate results

@@           Coverage Diff           @@
##           master   #16776   +/-   ##
=======================================
  Coverage   76.98%   76.98%           
=======================================
  Files        1007     1007           
  Lines       54186    54187    +1     
  Branches     7464     7465    +1     
=======================================
+ Hits        41713    41714    +1     
  Misses      12233    12233           
  Partials      240      240           

Flag	Coverage Δ
javascript	71.29% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...erset-frontend/src/datasource/DatasourceEditor.jsx	74.90% <100.00%> (+0.09%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a743526...a6cc029. Read the comment docs.

[villebro]

Merging this as-is, we can do a follow-up PR if we want to further refine this behavior.

[ktmud]

LGTM. Thanks for the quick fix!

For the case when the original temporal flag was inferred from db type instead of user selection, this change seems will also retain the flag, but maybe that's OK?

Yeah, I didn't think that would be a common need, but I'm open to changing that, too, if this is a typical scenario. Do you think it's common to want to disable temporal flags for temporal type columns?

I was more thinking of when some column changed from a datetime type to non datetime, the previously inferred is_dttm will be retained. But like I said, I think this case is also OK. It's much more likely to want to force a non-dttm to dttm than vice versa.

[villebro]

I was more thinking of when some column changed from a datetime type to non datetime, the previously inferred is_dttm will be retained. But like I said, I think this case is also OK. It's much more likely to want to force a non-dttm to dttm than vice versa.

CI seems to be so flaky we still have some time to iterate.. :D

I wonder if this would be the best flow: If the column exists and the raw datatype has changed, reinfer is_dttm. Therefore, if the raw datatype is unchanged, is_dttm is also unchanged.

This would have the drawback of disabling is_dttm when VARCHAR(10) is changed to VARCHAR(11) or TEXT or some other non-temporal type, but I assume this is fairly uncommon.