Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 211 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
# MindsDB — AI Coding Agent Guidelines

## Development Environment

- **Hot-reload**: MindsDB runs in Docker with a bind mount (`./mindsdb:/mindsdb`) and `watchfiles`. Python file changes take effect immediately — no container restart needed.
- **Testing queries**: Use `mindsdb_sdk` connecting to `http://127.0.0.1:47334`.
- **Config**: `config.json` at the project root, mounted at `/root/mindsdb_config.json` in the container.
- **Environment variables**: see `.env` (do not commit secrets; `GOOGLE_API_KEY` and DB credentials live there).

---

## Handler Architecture

### How the query planner splits API handler queries

When a handler is registered with `class_type = "api"`, MindsDB's query planner splits every SELECT into two steps:

1. **`FetchDataframeStep`** → calls the handler's `select()` with the original query (including complex targets). The handler must return the raw DataFrame with the columns DuckDB will need.
2. **`SubSelectStep`** → DuckDB executes the full original SELECT expression (CASE WHEN, SUM, GROUP BY, etc.) on top of the DataFrame from step 1.

**Implication**: handlers do not need to implement aggregations, CASE WHEN, or arithmetic. They only need to return the right raw columns. DuckDB handles everything else.

---

## Handler `select()` — Two Patterns

### Pattern A: Data-fetch-and-filter (most handlers)

The handler fetches all data from the API and then drops columns that weren't requested. Calendar, Search Console, email, HubSpot, Shopify, Xero all use this pattern.

**Correct implementation:**

```python
selected_columns = []
for target in query.targets:
if isinstance(target, ast.Star):
selected_columns = self.get_columns()
break
elif isinstance(target, ast.Identifier):
selected_columns.append(target.parts[-1])
else:
# Complex expression (CASE WHEN, SUM, BinaryOperation, etc.).
# The outer SubSelectStep/DuckDB layer handles the computation.
# Return all raw columns so DuckDB has what it needs.
selected_columns = self.get_columns()
break
if not selected_columns:
selected_columns = self.get_columns()
```

**Bugs to avoid:**
- `raise ValueError(f"Unknown query target {type(target)}")` — breaks any CTE or aggregation query.
- Silently skipping non-Identifier targets without a fallback — `selected_columns` stays empty and `set(df.columns).difference(set([]))` drops every column, returning an empty DataFrame.

### Pattern B: Column-selection-determines-API-params (e.g., Google Analytics)

The handler uses the SELECT targets to decide *what* to request from the API (GA4 dimensions vs metrics, Search Console dimensions, etc.). A raw `isinstance(target, ast.Identifier)` check silently skips columns referenced inside complex expressions, causing the API to be called with incomplete parameters.

**Correct implementation — add a recursive `_collect_identifiers` helper before the table class:**

```python
from typing import List
from mindsdb_sql_parser import ast


def _collect_identifiers(node) -> List[str]:
"""Recursively collect all Identifier column names from any AST node.

Walks into CASE WHEN, Function args, BinaryOperation, etc. so that
columns referenced inside complex expressions are not missed.
"""
if node is None:
return []
if isinstance(node, ast.Identifier):
return [str(node.parts[-1])]
if isinstance(node, ast.Case):
names = []
for condition, result in node.rules:
names.extend(_collect_identifiers(condition))
names.extend(_collect_identifiers(result))
names.extend(_collect_identifiers(node.default))
return names
if isinstance(node, ast.Function):
names = []
for arg in (node.args or []):
names.extend(_collect_identifiers(arg))
return names
if isinstance(node, ast.BinaryOperation):
return _collect_identifiers(node.args[0]) + _collect_identifiers(node.args[1])
if isinstance(node, ast.UnaryOperation):
return _collect_identifiers(node.args[0])
if isinstance(node, ast.TypeCast):
return _collect_identifiers(node.arg)
return []
```

**Then use it in `select()`:**

```python
seen = set()
for target in query.targets:
if isinstance(target, ast.Star):
# fall back to default dimensions/metrics
break
for col_name in _collect_identifiers(target):
if col_name in seen:
continue
seen.add(col_name)
# classify col_name as dimension or metric and add to API params
```

---

## Query Planner — Known Bugs Fixed in This Codebase

### 1. CTE must be cleared after `plan_cte()` — `query_planner.py`

After `self.plan_cte(query)` decomposes CTEs into steps, `query.cte` must be set to `None`. Otherwise the outer SELECT (which may reference a CTE name that resolves to a handler table) carries the full CTE definition into DuckDB, which fails with:

> `Catalog Error: Table with name <handler_table> does not exist`

```python
if query.cte is not None:
self.plan_cte(query)
query.cte = None # CTEs decomposed into steps; clear so DuckDB doesn't re-execute them
```

### 2. `plan_api_db_select` must NOT forward `order_by` to the handler — `query_planner.py`

`plan_api_db_select` splits a query into a handler fetch (`FetchDataframeStep`) and a DuckDB pass (`SubSelectStep`). It passes `order_by` from the SQL query to the handler, which is wrong: ORDER BY may reference **SQL aliases** (e.g. `SUM(sessions) AS total_sessions` → `ORDER BY total_sessions`) that are meaningless to the underlying API. The GA4 API returns:

> `400 Field total_sessions exists in OrderBy but is not defined in input Dimensions/Metrics list`

The outer SubSelectStep already retains `order_by` (it is not cleared like `where`/`limit`), so DuckDB applies it correctly after aggregation.

```python
# query_planner.py — plan_api_db_select()
query2 = Select(
targets=query.targets,
from_table=query.from_table,
where=query.where,
# order_by intentionally omitted: ORDER BY may reference SQL aliases unknown
# to the underlying API. The SubSelectStep/DuckDB layer handles it correctly.
limit=query.limit,
)
```

### 3. JOIN `filter_col_names` must only exclude scalar filters — `plan_join.py`

In `process_table()`, `filter_col_names` is computed to strip API filter parameters (e.g., `start_date = 'yesterday'`) from the SELECT column list so they are not sent to the API as dimensions. However, `get_filters_from_join_conditions()` also generates cross-table IN predicates (`page_segment IN (VALUES FROM t1)`), and naively extracting every `Identifier` from every condition will incorrectly remove JOIN dimension columns from the SELECT list, causing the right-hand table in a FULL OUTER JOIN to be missing those columns.

**Only exclude a column when the condition partner is a `Constant`:**

```python
filter_col_names = set()
for cond in conditions:
if isinstance(cond, BinaryOperation) and len(cond.args) >= 2:
# Only exclude scalar filter parameters (col = Constant).
# Do NOT exclude JOIN predicates (col IN (Parameter)).
for i, arg in enumerate(cond.args[:2]):
if isinstance(arg, Identifier):
other = cond.args[1 - i]
if isinstance(other, Constant):
filter_col_names.add(arg.parts[-1])
fetch_cols = referenced_cols - filter_col_names
```

---

## Handler Checklist

When creating or modifying a handler's `select()` method:

- [ ] Does the handler use target columns to control API parameters (Pattern B)?
- If yes: use `_collect_identifiers()` to recursively extract column names.
- [ ] Does the handler fetch all data and then filter by column (Pattern A)?
- If yes: add `else: selected_columns = self.get_columns(); break` and a `if not selected_columns: selected_columns = self.get_columns()` guard.
- [ ] Never `raise ValueError` on unrecognised target types — complex expressions are valid inputs from the planner.
- [ ] Never leave `selected_columns` empty after the targets loop — that silently drops all result columns.
- [ ] WHERE filter params (e.g., `start_date`, `end_date`) should be extracted from `query.where` and passed to the API, not treated as SELECT dimensions.
- [ ] `get_columns()` must list every column the API can return so Pattern A drop-logic works correctly.

---

## Handlers in This Project

| Handler | Pattern | Notes |
|---|---|---|
| `google_analytics_handler` | B | Uses `_collect_identifiers`; target columns map to GA4 dimensions/metrics |
| `google_calendar_handler` | A | Fetches all events/calendars/free-busy, then filters columns |
| `google_search_handler` | A | Fetches traffic/sitemaps/url-inspection data, then filters columns |
| `email_handler` | A (via `SELECTQueryParser`) | Delegated to utility — safe |
| `hubspot_handler` | A (via `SELECTQueryParser`) | Delegated to utility — safe |
| `shopify_handler` | A (via `SELECTQueryParser`) | Delegated to utility — safe |
| `xero_handler` | A | No target iteration — safe |
| `ms_one_drive_handler` | A | String checks only — safe |
| `web_handler` (`url_reader`) | A | Uses `FilterCondition`, no target iteration — safe |
| `s3_handler` | A | Only scans targets for `"content"` key; full query passed to DuckDB |

---

## Relevant Source Paths

| File | Purpose |
|---|---|
| `mindsdb/api/executor/planner/query_planner.py` | `plan_select`, `plan_cte`, `plan_api_db_select`, `get_integration_select_step` |
| `mindsdb/api/executor/planner/plan_join.py` | `PlanJoinTablesQuery`, `process_table`, `get_filters_from_join_conditions` |
| `mindsdb/api/executor/sql_query/steps/subselect_step.py` | `SubSelectStepCall` — runs DuckDB on handler result |
| `mindsdb/api/executor/utilities/sql.py` | `query_df`, `query_df_with_type_infer_fallback` |
| `mindsdb/integrations/utilities/query_traversal.py` | `query_traversal` — AST walker used across planner and handlers |
| `mindsdb/integrations/handlers/<name>/` | Individual handler implementations |
72 changes: 64 additions & 8 deletions mindsdb/api/executor/planner/plan_join.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ def __init__(self, planner):

self.step_stack = None
self.query_context = {}
self.table_columns = {} # TableInfo -> set of column names

self.partition = None

Expand Down Expand Up @@ -159,7 +160,12 @@ def resolve_table(self, table):
integration = self.planner.default_namespace

if integration is None and not hasattr(table, "sub_select"):
raise PlanningException(f"Database not found for: {table}")
available_dbs = ", ".join(list(self.planner.databases)[:10])
raise PlanningException(
f"Database not found for table: {table}.\n"
f"Available databases: {available_dbs}\n"
"Hint: Use format 'database_name.table_name'"
)

sub_select = getattr(table, "sub_select", None)

Expand Down Expand Up @@ -240,7 +246,11 @@ def check_node_condition(self, node):

table_info = self.get_table_for_column(arg1)
if table_info is None:
raise PlanningException(f"Table not found for identifier: {arg1.to_string()}")
known = ['.'.join(a) for t in self.tables for a in t.aliases]
raise PlanningException(
f"Table not found for identifier: {arg1.to_string()}.\n"
f"Known tables/aliases: {', '.join(known)}"
)

# keep only column name
arg1.parts = [arg1.parts[-1]]
Expand Down Expand Up @@ -308,13 +318,34 @@ def replace_subselects(node, **args):
# get all join tables, form join sequence
join_sequence = self.get_join_sequence(query.from_table)

# Collect column references per table from SELECT targets and JOIN ON
# conditions only (not WHERE — those are pushed down as filter params).
# This ensures API-type handlers fetch the right columns.
def _collect_fetch_columns(node, is_table, **kwargs):
if not is_table and isinstance(node, Identifier) and len(node.parts) > 1:
table_info = self.get_table_for_column(node)
if table_info is not None:
table_id = id(table_info)
if table_id not in self.table_columns:
self.table_columns[table_id] = set()
self.table_columns[table_id].add(node.parts[-1])

query_traversal(query.targets, _collect_fetch_columns)
for tbl in self.tables:
if tbl.join_condition is not None:
query_traversal(tbl.join_condition, _collect_fetch_columns)

# find tables for identifiers used in query
def _check_identifiers(node, is_table, **kwargs):
if not is_table and isinstance(node, Identifier):
if len(node.parts) > 1:
table_info = self.get_table_for_column(node)
if table_info is None:
raise PlanningException(f"Table not found for identifier: {node.to_string()}")
known = [str(a) for t in self.tables for a in t.aliases]
raise PlanningException(
f"Table not found for identifier: {node.to_string()}.\n"
f"Known tables/aliases: {', '.join(known)}"
)

# # replace identifies name
col_parts = list(table_info.aliases[-1])
Expand Down Expand Up @@ -391,15 +422,37 @@ def process_table(self, item, query_in):
table = copy.deepcopy(item.table)
table.parts.insert(0, item.integration)
table.is_quoted.insert(0, False)
query2 = Select(from_table=table, targets=[Star()])
# parts = tuple(map(str.lower, table_name.parts))

# Build WHERE conditions first (from query WHERE + JOIN ON filter params)
conditions = item.conditions
if "or" in self.query_context["binary_ops"]:
# not use conditions
conditions = []

conditions += self.get_filters_from_join_conditions(item)

# Use specific columns from SELECT targets and JOIN ON conditions.
# Exclude columns that are filter parameters (pushed as WHERE conditions)
# so API handlers (e.g. Google Analytics) don't try to use filter params
# like start_date/end_date as SELECT dimensions.
referenced_cols = self.table_columns.get(id(item))
if referenced_cols:
filter_col_names = set()
for cond in conditions:
if isinstance(cond, BinaryOperation) and len(cond.args) >= 2:
# Only exclude columns that are scalar filter parameters (col = Constant).
# Do NOT exclude columns used in cross-table JOIN predicates
# (col IN (Parameter)), as those columns must still be fetched.
for i, arg in enumerate(cond.args[:2]):
if isinstance(arg, Identifier):
other = cond.args[1 - i]
if isinstance(other, Constant):
filter_col_names.add(arg.parts[-1])
fetch_cols = referenced_cols - filter_col_names
targets = [Identifier(parts=[col]) for col in fetch_cols] if fetch_cols else [Star()]
else:
targets = [Star()]

query2 = Select(from_table=table, targets=targets)

if self.query_context["use_limit"]:
order_by = None
if query_in.order_by is not None:
Expand Down Expand Up @@ -493,7 +546,10 @@ def _check_conditions(node, **kwargs):
arg1, arg2 = arg2, arg1

if isinstance(arg2, Constant):
conditions.append(node)
conditions.append(copy.deepcopy(node))
# Neutralize in the join condition tree so DuckDB doesn't
# try to resolve filter params as DataFrame columns.
node.args = [Constant(0), Constant(0)]
elif table2 is not None:
data_conditions.append([arg1, arg2])

Expand Down
Loading
Loading