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

PLT-8339: Fix SQL instances for ChainPoint and ChainTip #224

Merged
merged 9 commits into from
Nov 1, 2023

Conversation

ana-pantilie
Copy link
Contributor

@ana-pantilie ana-pantilie commented Oct 31, 2023

Both ChainPoint and ChainTip can refer to the genesis block, in this case they don't carry any data. This should be reflected in the SQL representation correctly, therefore we need to make the changes in this PR.

@berewt pointed out the tests in

propSQLFieldRoundtripBlockHeaderHash :: Property
, unfortunately I wasn't able to write similar ones for these changes. The reason is that these would be tests for the RowParser, and the SQL package we use doesn't seem to have a way to run this parser in a pure context. I am 100% open to suggestions, let me know if there's something I'm missing here!

Also, there doesn't seem to be a way to return errors from the RowParser context, so I had to throw exceptions. Please let me know if we need to do something else here.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense and have useful messages
    • Important changes are reflected in changelog.d of the affected packages
    • Relevant tickets are mentioned in commit messages
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description
    • Targeting main unless this is a cherry-pick backport
    • Self-reviewed the diff
    • Useful pull request description
    • If relevant, reference the ADR in the PR and reference the PR in the ADR
    • Reviewer requested

@ana-pantilie ana-pantilie changed the title PLT-8339: Parse possible NULL values for ChainPoint PLT-8339: Fix SQL instances for ChainPoint and ChainTip Oct 31, 2023
@ana-pantilie ana-pantilie marked this pull request as ready for review October 31, 2023 13:06
@ana-pantilie ana-pantilie requested a review from berewt October 31, 2023 14:00
Copy link
Contributor

@berewt berewt left a comment

Choose a reason for hiding this comment

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

LGTM.
It's ok to throw errors, we catch them anyway https://github.com/input-output-hk/marconi/blob/main/marconi-core/src/Marconi/Core/Indexer/SQLiteIndexer.hs#L213-L220
We could maybe unwrap the RowParser using an internal module, but we can skip it at the moment.

@ana-pantilie ana-pantilie merged commit 1b6a439 into main Nov 1, 2023
3 checks passed
@ana-pantilie ana-pantilie deleted the PLT-8339-fix-chainpoint-when-querying branch November 1, 2023 11:45
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.

3 participants