Skip to content

Conversation

@Gilbert09
Copy link
Member

Problem

Changes

  • Look up the table via hogql instead - the same as we do in other places

@Gilbert09 Gilbert09 requested review from a team and Copilot November 27, 2025 16:19
Copilot finished reviewing on behalf of Gilbert09 November 27, 2025 16:22
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug where data warehouse table lookups were failing when referenced by their HogQL name instead of their database name. The fix aligns the lookup logic in update_paths_from_query with the existing pattern used in get_or_create_query_parent_paths.

Key changes:

  • Modified the table lookup logic to use HogQL database's get_table() method first
  • Added conditional logic to query by table_id when available, falling back to name otherwise
  • Ensures consistency with the existing lookup pattern used elsewhere in the same file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@Gilbert09 Gilbert09 enabled auto-merge (squash) November 28, 2025 14:43
.get()
)
else:
parent_table = (
Copy link
Contributor

Choose a reason for hiding this comment

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

in what situation a DataWarehouseTable would not have id?

@Gilbert09 Gilbert09 merged commit 4b968b5 into master Nov 28, 2025
174 checks passed
@Gilbert09 Gilbert09 deleted the tom/modeling-table-get branch November 28, 2025 14:54
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