Skip to content

Conversation

@jycor
Copy link
Contributor

@jycor jycor commented Dec 27, 2023

This PR has our behavior surrounding NOW() functions more closely match MySQL.

Changes:

  • Added NOW() synonyms to registry
  • Have CURRENT_TIMESTAMP(), LOCALTIME(), LOCALTIMESTAMP() all just call NOW()
  • Support parsing synonyms in DEFAULT and ON UPDATE expressions
  • Fixed SHOW CREATE TABLE to print CURRENT_TIMESTAMP for NOW() and synonyms

Companion PR: dolthub/vitess#296
Fixes:

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the biggest thing I'm noticing is that we could frontload some of the precision resolving. Not blocking though

// Must receive integer
// Should syntax error before this; check anyway
var fsp int
switch p := prec.(type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lot of overlap with the default resolving, can we push all of this into planbuilder when resolving now functions? It might be easy to resolve as an integer type upfront also, one less expression to track through analysis

Copy link
Contributor Author

@jycor jycor Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there are other functions, like CURRENT_TIME and UTC_TIMESTAMP, that should be using similar logic. Will do this refactor in a follow up PR.

@jycor jycor merged commit 21534da into main Jan 3, 2024
@jycor jycor deleted the james/now-siblings branch January 3, 2024 07:30
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