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

feat(temporal data): add time slice key to conflict clause #249

Merged
merged 8 commits into from
Nov 16, 2023

Conversation

sjvans
Copy link
Contributor

@sjvans sjvans commented Sep 25, 2023

  • test
  • changelog will happen automatically once merged

@nocin
Copy link

nocin commented Oct 5, 2023

It would be great if this fix could make it in the next release. This would simplify my life a lot! Thanks!

Copy link
Member

@patricebender patricebender left a comment

Choose a reason for hiding this comment

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

@sjvans Could you add a test which shows the effects of this change?

@BobdenOs BobdenOs requested a review from stewsk as a code owner October 17, 2023 10:50
@nocin
Copy link

nocin commented Nov 2, 2023

Hi all,

thanks for moving this topic forward! Am I right, that only one check is missing until it could be merged? That would be awesome!

BR, Nico

@johannes-vogel
Copy link
Contributor

Hi all,

thanks for moving this topic forward! Am I right, that only one check is missing until it could be merged? That would be awesome!

BR, Nico

@nocin Is it an option for you to make @cds.valid.from element a key? That should already work today.

@nocin
Copy link

nocin commented Nov 3, 2023

Hi all,
thanks for moving this topic forward! Am I right, that only one check is missing until it could be merged? That would be awesome!
BR, Nico

@nocin Is it an option for you to make @cds.valid.from element a key? That should already work today.

Hi @johannes-vogel,

thanks for the hint! I now spend some time thinking about this approach, but it would mean a lot of work in backend, frontend and another external program which pushes data in the application, as in our case it is the "main" table of the project. Therefore, I would prefer to stick to a single UUID as key and will wait for this feature, until I continue working on my feature request.

BR, Nico

@johannes-vogel johannes-vogel added the next release pr to be checked for next release label Nov 15, 2023
}

const val = _managed[element[annotation]?.['=']]
|| !isUpdate && (element['@cds.valid.from'] ? '$valid.from' : element['@cds.valid.to'] ? '$valid.to' : null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is || !isUpdate && (element['@cds.valid.from'] ? '$valid.from' : element['@cds.valid.to'] ? '$valid.to' : null) required?

Copy link
Contributor Author

@sjvans sjvans Nov 16, 2023

Choose a reason for hiding this comment

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

otherwise you cannot insert/ upsert data without specifying both validFrom and validTo in the payload.
in other words:
coalesce(ISO(value->>'$."validFrom"'), session_context('$valid.from'))
instead of just
ISO(value->>'$."validFrom"').

Copy link
Contributor Author

@sjvans sjvans Nov 16, 2023

Choose a reason for hiding this comment

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

@BobdenOs @johannes-vogel
was the default validTo changed? i now get $now but i would have expected something like 9999-12-31T23:59:59.999Z. with $now this defaulting logic is not good.

Copy link
Contributor

Choose a reason for hiding this comment

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

session_context('$now') will return the timestamp from the tx.timestamp. Or is the $now part of the entries || data ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new Date().toISOString().replace(/(\dZ?)$/, d => parseInt(d[0]) + 1 + d[1] || ''))
defaults to $now (logically)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean. I don't know what the behavior used to be.
I am also not sure whether the max timestamp default is correct in the temporal context as I would expect that any new insert with the same key values should become the validTo of the last value.

await INSERT({ID: 1/*, validFrom: $now, validTo: timestamp_max */}).into(temporal)
await sleep(...)
await Promise.all([
  INSERT({ID:1/*, validFrom: $now, validTo: timestamp_max */}).into(temporal),
  UPDATE(temporal).data({validTo: $now}).where(`ID = ${1} and validTo = ${timestamp_max}`),
])

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we did not support/document how to write temporal data so far.
For now I think it's fine that the input is required by the user → #344

@sjvans sjvans enabled auto-merge (squash) November 16, 2023 11:27
@sjvans sjvans merged commit 67b8edf into main Nov 16, 2023
@sjvans sjvans deleted the temporal-data branch November 16, 2023 11:29
@@ -874,7 +874,6 @@ class CQN2SQLRenderer {
if (converter && sql[0] !== '$') sql = converter(sql, element)

let val = _managed[element[annotation]?.['=']]
|| !isUpdate && (element['@cds.valid.from'] ? '$valid.from' : element['@cds.valid.to'] ? '$valid.to' : null)
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look correct. If the session context contains the from and to it should be considered.

@cap-bots cap-bots mentioned this pull request Nov 15, 2023
@cap-bots cap-bots mentioned this pull request Dec 5, 2023
@cap-bots cap-bots mentioned this pull request Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release pr to be checked for next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants