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

Feature/translate json functions #29306 #30010

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joelmandell
Copy link

@joelmandell joelmandell commented Jan 9, 2023

Fixes #29306

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format: (This I totally forgot)
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo (Kinda... little bit different with unit-tests. But want to get your feedback if that's okay).

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks for this PR - it's a good start, see comments below.

Note that there are some other JSON functions supported by SQL Server (ISJSON, JSON_ARRAY, JSON_OBJECT...). Ideally we'd have translations for all of them (and the relevant SQLite ones as well); of course you don't have to do that in this PR, but if you feel like working on those that would be great.

Copy link
Author

@joelmandell joelmandell left a comment

Choose a reason for hiding this comment

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

.

Copy link
Author

@joelmandell joelmandell left a comment

Choose a reason for hiding this comment

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

I made some changes now. And think I got them all covered. But little bit uncertain about the propagation of null.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

@joelmandell sorry, it's taking long for me to get around to this, here are some comments. Please rebase this PR on the latest main and check out any test failures after that.

// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.EntityFrameworkCore.TestModels.JsonQuery;
public class JsonEntityBasicString
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to add this rather than simply using JsonEntityBasic?

Copy link
Author

@joelmandell joelmandell Jul 15, 2023

Choose a reason for hiding this comment

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

JsonEntityBasic has mappings of the columns to classes, and this works only for strings If I understand it correctly.

@joelmandell
Copy link
Author

@roji I know I was late to the party and missed to tag you in to this before feature freeze for 8.0... Do you have any advice or comments on how I can move forward with this one to perhaps get it ready for 9.0? I can as well look into the other requests for json functions if possible.

@roji
Copy link
Member

roji commented Nov 12, 2023

@joelmandell sure thing - I'd also definitely like us to get this in for 9.0.

Here's my advice... Give us around a month, maybe a bit more to deal with the 8.0 release and the subsequent patching and other work - and then ping me here again. Assuming it's a good time to iterate on it, you can then rebase this PR on the latest main, and then we can get this to completed state and merge.

/// </summary>
public SqlExpression? Translate(
SqlExpression? instance,
MethodInfo method,
Copy link

@Abdurahmon0412 Abdurahmon0412 Nov 14, 2023

Choose a reason for hiding this comment

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

You must resolve conflict

@joelmandell
Copy link
Author

Ping @roji. I do not know if it is to early to continue on this one now? :)

@roji roji force-pushed the feature/translate_json_functions branch from ee39973 to 354a9df Compare December 19, 2024 15:31
@roji roji requested a review from a team as a code owner December 19, 2024 15:31
@roji
Copy link
Member

roji commented Dec 19, 2024

@joelmandell I'm looking at this PR again (sorry for all the breaks and pauses). Since there was previously some problematic merge commit which messed up the entire history, I've squashed everything cleanly rebased on top of the latest main. In the interest of finally getting moving quickly and finally getting this merge, I'll do whatever fixup is still necessary and push commits on this PR.

@roji roji unassigned maumar Dec 19, 2024
@joelmandell
Copy link
Author

@roji Sounds good. Thx

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.

Provide EF.Functions translations for JSON functions
7 participants