Add account and transaction graph views - #2
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c47ae60ef0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| combined = pl.concat_str( | ||
| [pl.col(date_column).cast(pl.String), pl.col(column)], | ||
| separator=" ", | ||
| ).str.to_datetime(strict=False) |
There was a problem hiding this comment.
When a table has a string time column and a date column already parsed as pl.Datetime—including tables passed through clean_lazy_frame—casting the date to a string includes its midnight time, so this constructs values such as 2025-01-01 00:00:00 09:30:00. The strict-false parser turns these into nulls, and build_transaction_graph then either drops every row or raises that no timestamps are parseable. Extract or format only the calendar date before concatenating it with the time column.
Useful? React with 👍 / 👎.
| frame = frame.with_columns( | ||
| pl.col(id_column).cast(pl.String).alias("transaction_id") | ||
| ) |
There was a problem hiding this comment.
When the selected transaction-ID column contains nulls or duplicate values, this cast accepts them unchanged even though transaction-graph edges identify nodes exclusively through transaction_id. Multiple transaction rows consequently become indistinguishable in source_transaction_id and target_transaction_id, so downstream graph construction can conflate distinct transactions or create null endpoints. Validate uniqueness/non-nullness or generate stable replacement IDs before building the graph.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5e9d09cff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if date_column is not None and date_column != column: | ||
| date_only = pl.col(date_column).cast(pl.String).str.replace( | ||
| r"[T ].*$", "" | ||
| ) | ||
| return pl.concat_str( |
There was a problem hiding this comment.
When the table contains both a separate Date column and a selected string timestamp that already includes a date, this branch unconditionally produces values such as 2025-01-02 2025-01-01 09:00. Parsing then yields nulls, so build_transaction_graph can reject an otherwise valid table as containing no parseable timestamps. Attempt direct timestamp parsing first and only combine with Date when the selected values are time-only.
Useful? React with 👍 / 👎.
| "account metadata ID", | ||
| ) | ||
| metadata = ( | ||
| metadata.with_columns(pl.col(id_column).cast(pl.String).alias("node_id")) |
There was a problem hiding this comment.
When account metadata identifiers contain leading or trailing whitespace, their attributes fail to join because transaction endpoints are stripped in _prepare_transactions while metadata IDs are only cast to strings here. This leaves metadata columns null for nodes that otherwise represent the same account; apply the same whitespace normalization to the metadata key before deduplicating and joining.
Useful? React with 👍 / 👎.
pigu163
left a comment
pigu163
left a comment
There was a problem hiding this comment.
@codex Please review the latest changes in this PR only for graph-construction correctness and test coverage.
Focus on:
AccountGraph: accounts are nodes, transactions are directed edges, and repeated transactions between the same accounts are preserved.TransactionGraph: createTi -> Tjonly when:receiver(Ti) == sender(Tj)timestamp(Tj) > timestamp(Ti)timestamp(Tj) - timestamp(Ti) <= delta
- Do not connect transactions merely because they share an account.
- One transaction may connect to multiple valid future transactions.
- Check that
via_accountandtime_deltaare constructed correctly. - Check that the implementation avoids obviously incorrect pairwise graph construction logic.
- Check whether tests adequately cover:
- repeated account edges
- correct transaction succession
- wrong shared-account cases
- backward time
- same timestamp
- delta exceeded
- exact delta boundary
- multiple successors
Please report any correctness issues or missing tests, with file/function references.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d2266edf4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ] | ||
| nodes = frame.select(node_columns) | ||
| ordered = frame.sort(["timestamp", _ROW_INDEX]) | ||
| rows = list(ordered.iter_rows(named=True)) |
There was a problem hiding this comment.
When the input uses Polars nanosecond timestamps, iter_rows converts them to Python datetime values and truncates precision to microseconds. For example, matching transactions 500 ns apart with a 1 µs delta become equal here, so bisect_right incorrectly rejects the valid successor; other sub-microsecond differences also produce inaccurate time_delta values. Keep comparisons and differences in Polars or integer timestamp units rather than Python datetimes.
Useful? React with 👍 / 👎.
| ] | ||
| ) | ||
|
|
||
| graph = build_transaction_graph(frame, delta=timedelta(hours=1)) |
There was a problem hiding this comment.
This core test includes matching successors 20, 30, and exactly 60 minutes away, but none beyond the one-hour window. Consequently, an implementation that omitted the upper-bound check entirely would still pass the suite. Add a matching future transaction more than one hour after its predecessor and assert that no edge is created.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bce04c2774
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if timestamp_column is not None or _has_timestamp_column(frame.columns): | ||
| timestamp = _resolve_required_column( | ||
| frame.columns, | ||
| timestamp_column, | ||
| _TIMESTAMP_ALIASES, | ||
| "timestamp", | ||
| ) | ||
| frame = frame.with_columns( | ||
| _timestamp_expression(frame, timestamp).alias("timestamp") |
There was a problem hiding this comment.
When build_account_graph receives a string timestamp outside _parse_datetime_strings' hard-coded YYYY-MM-DD formats—for example the raw IBM value 2022/09/01 00:20—this shared preparation path runs even though an account graph does not require timestamps and overwrites the valid attribute with null. This silently corrupts edge metadata; timestamp parsing should only be required for transaction graphs, or the original value should be retained when parsing fails.
Useful? React with 👍 / 👎.
|
To use Codex here, create an environment for this repo. |
1 participant
No description provided.