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

Are multi column joins no longer supported? #77

Open
travis-leith opened this issue Dec 15, 2022 · 9 comments
Open

Are multi column joins no longer supported? #77

travis-leith opened this issue Dec 15, 2022 · 9 comments

Comments

@travis-leith
Copy link

This feature was discussed and implemented: #62
and tests were written: https://github.com/Dzoukr/Dapper.FSharp/pull/63/files#diff-7a8dde6aa28f58c34432379d8cfa27c1b7ec21029d248424056110332d7cd49f
But I no longer see any similar tests

@travis-leith
Copy link
Author

I changed to version 3.4 but I am still not able to get this to work. Here is my syntax

            select {
                for o in leftTable do
                //leftJoin v in rightTable on (o.Id = v.OrderId)
                leftJoin v in rightTable on ((o.Id, DateTimeOffset.MaxValue) = (v.OrderId, v.ValidTo))
                selectAll
            }

working against sql server. The error I am getting is One or more errors occurred. (The given key was not present in the dictionary.)

The commented out left join works fine btw. Am I missing something?

@Dzoukr
Copy link
Owner

Dzoukr commented Dec 16, 2022

Can you try the latest version, please? It should be still supported 🤔

Maybe, if it doesn't work, can you write a simple test and make PR, I will try to have a look at it?

@travis-leith
Copy link
Author

I have forked/cloned but I cannot run the tests because the only Sql server database I have access to is Azure SQL Database, which does not support switching databases in a connection. See here (https://social.msdn.microsoft.com/Forums/en-US/1d83c593-095d-416b-b927-7a6489503131/sql-quotusequot-statement-is-not-supported-in-azure-sql-database-for-selecting-different?forum=ssdsgetstarted)

So the tests are not compatible with Azure Sql DB. I can try to adjust all the MsSql tests so that they use a dedicated init connection for set up, then use a different connection to run the tests. Not sure if this would be acceptable to you.

@Dzoukr
Copy link
Owner

Dzoukr commented Dec 16, 2022

It's all based on Docker. 😃 You can run docker compose up -d and run all the tests easily.

See: https://github.com/Dzoukr/Dapper.FSharp/blob/master/docker-compose.yml

@travis-leith
Copy link
Author

I will have to wait until I get home to get access to a machine with docker, but in the meanwhile I have adjusted the test setup to work for Azure Sql DB and added the following test

[<Test>]
    member _.``select with multi column left join``() =
        task{
            do! init.InitPersons()
            do! init.InitDogs()

            let persons = Persons.View.generate 10
            let dogs = Dogs.View.generate1toN 5 persons.Head

            let! _ =
                insert {
                    into personsView
                    values persons
                } |> conn.InsertAsync
            let! _ =
                insert {
                    into dogsView
                    values dogs
                } |> conn.InsertAsync

            let! fromDb =
                select {
                    for p in personsView do
                    leftJoin d in dogsView on ((p.Id, "Dog 1") = (d.OwnerId, d.Nickname))
                    selectAll
                } |> conn.SelectAsyncOption<Persons.View, Dogs.View>

            Assert.AreNotEqual(fromDb |> Seq.length, 0)
        }

which passes just fine. So I have no idea what is wrong with my other code.

@travis-leith
Copy link
Author

aha! The problem is DateTimeOffset.MaxValue. If I replace this with a join on a string field in the tuple, it works just fine. Any thoughts?

@travis-leith
Copy link
Author

The following seems to work

            let maxDate = DateTimeOffset.MaxValue
            return! select {
                for o in leftTable do
                //leftJoin v in rightTable on (o.Id = v.OrderId)
                leftJoin v in rightTable on ((o.Id, maxDate) = (v.OrderId, v.ValidTo))
                selectAll
            }

not sure if you would consider this a bug or not. If not, might be worth documenting somehow.

@travis-leith
Copy link
Author

@Dzoukr I will leave it up to you to decide to close this issue or leave it open until the "bug" is either fixed or documented.

@JordanMarr
Copy link
Contributor

JordanMarr commented Dec 16, 2022

This would be an easy fix.

However, the F# join on syntax adds constraints that would make it impossible to express certain kinds of join filter clauses.

For example, what if you need to do this:

SELECT *
FROM leftTable o
LEFT JOIN rightTable v ON (o.Id = v.OrderId AND v.ValidTo > @maxDate)

Since F# doesn't support multiple clauses separated by AND or OR in the on statement, this would be impossible to express because the first clause is =, but the second clause is >.

For that reason, I would suggest getting into the habit of moving these filters to the where clause:

return! select {
    for o in leftTable do
    leftJoin v in rightTable on (o.Id = v.OrderId)
    where (v.ValidTo = DateTimeOffset.MaxValue)
    selectAll
}

Incidentally, the where handler is much more robust and would already accommodate the DateTimeOffset.MaxValue without having to capture it in a binding ahead of time.

In hindsight, we probably should have just made it a requirement to move join filtering to the where clause rather than adding partial support for doing it in the join statement (as was done due to a previous user request). Because it simply can't handle the full range of expressions required if going down that path.

In fact, it would probably be a good idea to steer users down that path in the documentation and downplaying tuple join clauses in general (even though they are partially supported), since at some point a user will have a more complex join filter that can only be handled that way.

But I'm glad you were able to find a solution. 💪

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

No branches or pull requests

3 participants