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

🐛 BUG: when trying to left join 2 tables that have same name columns - second column is not returned #3160

Open
AndriiSherman opened this issue May 7, 2023 · 24 comments · Fixed by #5917 or #5906
Assignees
Labels
bug Something that isn't working d1 Relating to D1

Comments

@AndriiSherman
Copy link

Which Cloudflare product(s) does this pertain to?

D1

What version of Wrangler are you using?

2.19.0

What operating system are you using?

macOS

Describe the Bug

As per issue #555 we can see, that d1 stopped returning duplicate names from different tables. It was working before and it's how any other driver is doing for raw mode

$ pnpm wrangler d1 execute dzltest --command='select "users"."id", "posts"."id", "posts"."ownerId", "posts"."title", "posts"."content" from "users" left join "posts" on "posts"."ownerId" = "users"."id";'

┌────┬─────────┬───────┬───────────┐
│ id │ ownerId │ title │ content   │
├────┼─────────┼───────┼───────────┤
│ 2  │ 1       │ test  │ 111111111 │
├────┼─────────┼───────┼───────────┤
│ 3  │ 1       │ ueoau │ ueueu     │
├────┼─────────┼───────┼───────────┤
│ 1  │ 1       │ xxx   │ yyy       │
└────┴─────────┴───────┴───────────┘

But should return id for user and id for posts as well
Expected

┌────┬────┬─────────┬───────┬───────────┐
│ id │ id │ ownerId │ title │ content   │
├────┼────┼─────────┼───────┼───────────┤
│ 2  │ 1  │    1    │ test  │ 111111111 │
├────┼────┼─────────┼───────┼───────────┤
│ 3  │ 2  │    1    │ ueoau │ ueueu     │
├────┼────┼─────────┼───────┼───────────┤
│ 1  │ 3  │    1    │ xxx   │ yyy       │
└────┴────┴─────────┴───────┴───────────┘
@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented May 8, 2023

👋🏻 Greetings,
I appreciate the report.

Feel free to check out the Cloudflare Discord where there is a channel for D1 as well: https://discord.com/invite/cloudflaredev

Cc: @rozenmd

@JacobMGEvans JacobMGEvans added the d1 Relating to D1 label May 8, 2023
@rozenmd
Copy link
Contributor

rozenmd commented May 15, 2023

@AndriiSherman could I bother you for a schema/db sql file to reproduce this?

@xdivby0
Copy link

xdivby0 commented May 15, 2023

@rozenmd Although I am not Andrii, I think I have the same problem and here's a minimal example schema that has the issue (pretty much any schema that you can join where both have one shared field name (here it's "id"):

import type { InferModel } from "drizzle-orm";
import {
  integer, primaryKey,
  sqliteTable,
  text
} from "drizzle-orm/sqlite-core";

export const users = sqliteTable(
  "users",
  {
    id: integer("id").primaryKey({
      autoIncrement: true
    }),
    email: text("email").notNull(),
  }
);

export const subscriptions = sqliteTable(
  "subscriptions",
  {
    id: integer("id").primaryKey({ autoIncrement: true }),
    user: integer("user").notNull().references(() => users.id, {
      onDelete: "cascade"
    })
  }
);

@AndriiSherman
Copy link
Author

Agreed. Any SELECT query involving joins and having the same fields in the response will not work as expected.

In our case, we are using the raw mode for querying, but it appears that the raw mode in the "d1" driver differs from the raw mode in other drivers

Link to a code: https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/templates/d1-beta-facade.js#L177

After receiving the response in JSON format, it is subsequently mapped to an array of arrays at the code level. Consequently, fields with the same name will be overridden.

@rozenmd
Copy link
Contributor

rozenmd commented May 24, 2023

We've traced this back to workerd, and will be discussing possible solutions here: cloudflare/workerd#696

Keep in mind @AndriiSherman that a temporary fix on Drizzle's side is still possible by using .all() and manipulating the data into the array of arrays format yourselves.

@itsmatteomanf
Copy link
Contributor

Keep in mind @AndriiSherman that a temporary fix on Drizzle's side is still possible by using .all() and manipulating the data into the array of arrays format yourselves.

I’m pretty sure it behaves wrongly even with this option. I’m gonna need to check, but how could it return multiple identical keys in an array of objects?

Normally SQL databases return the table name in the joined table’s column names. Or even in both.

Do you want me to check, even if it’s impossible to return multiple keys of the same name, @rozenmd?

@marina-chibizova
Copy link

We have the same problem, does anything block fix for this bug?

@elithrar
Copy link
Contributor

elithrar commented Aug 1, 2023

We are working on this. Current priority is larger databases, which is the biggest ask from most users.

@marina-chibizova
Copy link

Thanks for a prompt response @elithrar!

@jwhits
Copy link

jwhits commented Jan 7, 2024

We are working on this. Current priority is larger databases, which is the biggest ask from most users.

Is there any update on resolving this issue? I have tables with the same names for common fields like created_at, updated_at, id etc and joins don't work because all the response data is shifting columns in the results.

@geelen
Copy link
Contributor

geelen commented Jan 30, 2024

FYI, this will be fixed D1 once cloudflare/workerd#1586 is merged and rolls out

It will only fix the results of .raw(), .all() will still have later columns with the same name overwriting earlier ones.

@geelen
Copy link
Contributor

geelen commented Feb 13, 2024

This is now released. .raw() returns the full results set regardless of column name clashes. Sadly Wrangler still uses .all() so this won't be visible except from inside the Worker API, so I'll leave this ticket open for now.

@elithrar
Copy link
Contributor

Changelog: https://developers.cloudflare.com/d1/platform/changelog/#2024-02-13

cc @AndriiSherman on the Drizzle team.

@elithrar elithrar reopened this Feb 14, 2024
@AndriiSherman
Copy link
Author

Changelog: https://developers.cloudflare.com/d1/platform/changelog/#2024-02-13

cc @AndriiSherman on the Drizzle team.

Has it been released already? I would like to run some tests, can't find this version for types on npm

@rozenmd
Copy link
Contributor

rozenmd commented Feb 23, 2024

@AndriiSherman sorry for the delay there - the types are released in 4.20240222.0

@dankochetov
Copy link

@rozenmd I've tested this with latest types version (4.20240320.1), and the .raw() method still doesn't return multiple columns with the same name. This change shouldn't require any actions from our side, since we don't bundle @cloudflare/workers-types with Drizzle, it's used as a peer dependency, so it should be enough for users to just update their dependency version. However, it doesn't seem to be the case.

Here's a reproduction repo: https://github.com/dankochetov/drizzle-d1-bug
I've added a raw D1 call to make sure it wasn't Drizzle's fault, and observed the same behavior. As you could see from the logs, the columns list in the response don't match the columns list in the query.

image
image

@geelen
Copy link
Contributor

geelen commented Mar 24, 2024

Hi @dankochetov, that looks like a bug in local mode (probably related to getPlatformProxy(), though I'll need to dig in and figure out exactly what).

The .raw() behaviour has been fixed in production though, I've just converted your repro to actually publish a worker and I get the expected output:

image

I'll take a look at the local behaviour this week, apologies for the oversight.

@stefanmaric
Copy link

has been fixed in production though

@geelen I'm using the latest wrangler 3.37.0 to deploy my app with compatibility_date = "2024-03-25" and I'm still facing this issue.

Is there anything special I need to do to get the new version?

@stefanmaric
Copy link

stefanmaric commented Mar 25, 2024

Running my query from the D1 Dashboard console UI or wrangler D1 CLI get me the same, buggy result. It collapses columns with the same name from joined tables into a single one, the last joined table seemingly taking precedence.

SELECT id, A.description, B.description FROM table_a A LEFT JOIN table_b B ON B.id = A.id WHERE id = 'abc'

@F0rce
Copy link

F0rce commented Apr 19, 2024

Are there any updates on this?

kossnocorp added a commit to kossnocorp/workers-sdk that referenced this issue May 25, 2024
Miniflare squashes columns with the same name, which is inconsistent with expected behavior and production where the problem seemingly was fixed.
@kossnocorp
Copy link
Contributor

I added a test case for the problem: #5917

I went down a rabbit hole chasing this bug through workerd (where I also added tests, but it passed), and now, Workers SDK.

Hopefully someone from the maintainers will notice it and help me fix the problem. Any tips will be appriciated.

kossnocorp added a commit to kossnocorp/workerd that referenced this issue May 25, 2024
The issue: cloudflare/workers-sdk#3160

The test is passing and it is proof of the expected behavior that Workers SDK doesn't match.
kossnocorp added a commit to kossnocorp/workerd that referenced this issue May 25, 2024
The issue: cloudflare/workers-sdk#3160

The test is passing and it is proof of the expected behavior that Workers SDK doesn't match.
kossnocorp added a commit to kossnocorp/workers-sdk that referenced this issue May 25, 2024
This PR fixes cloudflare#3160 caused by ignoring the `resultsFormat` query that `workerd` sends.

The commits add proper handling of the query. It makes `raw` preserve columns with the same name by handling `resultsFormat=ROWS_AND_COLUMNS` and removes `results` from `run` as it sends `resultsFormat=NONE` (see https://github.com/cloudflare/workerd/blob/1d89f3b8e9cdcd898ea486656d72d9551e79f4a3/src/cloudflare/internal/d1-api.ts#L295)
@kossnocorp
Copy link
Contributor

I found the root cause of the problem and fixed it: #5917

Hopefully, the PR will be merged and shipped soon.

kossnocorp added a commit to kossnocorp/workers-sdk that referenced this issue May 27, 2024
This PR fixes cloudflare#3160 caused by ignoring the `resultsFormat` query that `workerd` sends.

The commits add proper handling of the query. It makes `raw` preserve columns with the same name by handling `resultsFormat=ROWS_AND_COLUMNS` and removes `results` from `run` as it sends `resultsFormat=NONE` (see https://github.com/cloudflare/workerd/blob/1d89f3b8e9cdcd898ea486656d72d9551e79f4a3/src/cloudflare/internal/d1-api.ts#L295)
@rozenmd
Copy link
Contributor

rozenmd commented May 27, 2024

@kossnocorp - that PR specifically fixes Workers when run locally, not the execute command. I'll have a follow-up PR for the execute command shortly.

rozenmd added a commit that referenced this issue May 27, 2024
* Handle resultsFormat in D1 worker

This PR fixes #3160 caused by ignoring the `resultsFormat` query that `workerd` sends.

The commits add proper handling of the query. It makes `raw` preserve columns with the same name by handling `resultsFormat=ROWS_AND_COLUMNS` and removes `results` from `run` as it sends `resultsFormat=NONE` (see https://github.com/cloudflare/workerd/blob/1d89f3b8e9cdcd898ea486656d72d9551e79f4a3/src/cloudflare/internal/d1-api.ts#L295)

* Handle resultsFormat in D1 worker

This PR fixes #3160 caused by ignoring the `resultsFormat` query that `workerd` sends.

The commits add proper handling of the query. It makes `raw` preserve columns with the same name by handling `resultsFormat=ROWS_AND_COLUMNS` and removes `results` from `run` as it sends `resultsFormat=NONE` (see https://github.com/cloudflare/workerd/blob/1d89f3b8e9cdcd898ea486656d72d9551e79f4a3/src/cloudflare/internal/d1-api.ts#L295)

* Update packages/miniflare/test/plugins/d1/test.ts

* Ignore NONE format behaviour in D1 worker

See comment: #5917 (comment)

* Add passing test for new bindings

* Update .changeset/little-jars-train.md

* Update .changeset/little-jars-train.md

* Update .changeset/little-jars-train.md

* Update .changeset/little-jars-train.md

* Update little-jars-train.md

---------

Co-authored-by: Max Rozen <3822106+rozenmd@users.noreply.github.com>
@rozenmd
Copy link
Contributor

rozenmd commented May 27, 2024

Ugh GitHub - nope not fixed yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working d1 Relating to D1
Projects
None yet