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

Pass through change updates to change compactor #304

Merged
merged 11 commits into from
Feb 6, 2025

Conversation

chowbao
Copy link
Contributor

@chowbao chowbao commented Jan 22, 2025

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with the jira ticket associated with the PR.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated the README with the added features, breaking changes, new instructions on how to use the repository. I updated the description of the fuction with the changes that were made.

Release planning

  • I've decided if this PR requires a new major/minor/patch version accordingly to
    semver, and I've changed the name of the BRANCH to major/_ , minor/_ or patch/* .

What

Github issue

Moving a copied version of the changeCompactor with the new Change struct fields into stellar-etl to pass through transaction_id, operation_id, and operation_type to ledger entry changes

Why

To get more useful join keys in the ledger entry change tables in BQ

Known limitations

  • transaction_id, operation_id, and operation_type may not be entirely correct because we are using compacted changes where we keep the data for the latest change within a ledger. This should be fixed when stellar-etl is moved off of using changeCompactor and the ledger entry change table saves all changes instead of compacted changes
  • Some of this plus simpler logic could be used depending on Update the ingest/change Change struct to always pass LCM if available go#5578

internal/transform/schema.go Outdated Show resolved Hide resolved
Copy link

socket-security bot commented Jan 22, 2025

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
golang/github.com/dgryski/[email protected] 🔁 golang/github.com/dgryski/[email protected] None 0 84.6 kB
golang/github.com/stellar/[email protected] 🔁 golang/github.com/stellar/[email protected] None 0 15.5 MB

View full report↗︎

go.mod Show resolved Hide resolved
internal/input/change_compactor.go Show resolved Hide resolved
internal/transform/schema.go Outdated Show resolved Hide resolved
@chowbao chowbao marked this pull request as ready for review January 23, 2025 02:12
@chowbao chowbao requested a review from a team as a code owner January 23, 2025 02:12
@chowbao
Copy link
Contributor Author

chowbao commented Jan 23, 2025

One other important note. This requires a schema change for the raw tables. I think it'd be worthwhile to have this run in the test airflow over the weekend if possible

@sydneynotthecity
Copy link
Contributor

I think it'd be worthwhile to have this run in the test airflow over the weekend if possible

Yup, agreed. I also think we should modify the dune exports and drop these new columns and determine whether or not it's necessary to propagate into dune. If our end goal is to create a token_transfers table there, i don't think it's worthwhile to add to external facing warehouses.

Copy link
Contributor

@sydneynotthecity sydneynotthecity left a comment

Choose a reason for hiding this comment

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

Looks good, i didn't review the golden files and just assume they implicitly work based on tests passing

internal/input/change_compactor.go Show resolved Hide resolved
internal/transform/config_setting.go Outdated Show resolved Hide resolved
internal/transform/trade_test.go Show resolved Hide resolved
internal/utils/main.go Show resolved Hide resolved
internal/utils/main.go Show resolved Hide resolved
@chowbao chowbao mentioned this pull request Jan 28, 2025
6 tasks
@chowbao chowbao merged commit c98062e into master Feb 6, 2025
8 checks passed
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