Skip to content

Switch map:merge from use-last to use-first as per XQuery spec #3738

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.

Sign up for GitHub

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

Jump to bottom
Merged
reinhapa merged 4 commits into from
Feb 6, 2023
Merged

Switch map:merge from use-last to use-first as per XQuery spec #3738

reinhapa merged 4 commits into from
Feb 6, 2023

Conversation

@adamretter
Copy link
Contributor

@adamretter adamretter commented Feb 18, 2021 •
edited
Loading

Closes #3750

Fixes map:merge in eXist-db so that It implements use-first as the default behaviour for as per the XQuery spec. Before this change eXist-db implemented use-last as the default.

@joewiz joewiz added enhancement new features, suggestions, etc. needs documentation Signals issues or PRs that will require an update to the documentation repo labels Feb 18, 2021
@joewiz joewiz mentioned this pull request Feb 18, 2021
Open
12 tasks
@line-o
Copy link
Member

line-o commented Feb 19, 2021

I do consider the switch from implicit use-last to use-first a breaking change.

@adamretter
Copy link
Contributor Author

adamretter commented Feb 19, 2021 •
edited
Loading

I do consider the switch from implicit use-last to use-first a breaking change.

@line-o It's a bugfix, but I agree it is also likely a breaking change for people who have relied on the non-spec compliant behaviour that eXist-db previously implemented... so 6.0.0 then?

@line-o
Copy link
Member

line-o commented Mar 3, 2021

Saxon has changed the behaviour of map:merge quite recently 9.7.0.7 -> 9.7.0.8 https://www.saxonica.com/documentation/index.html#!functions/map/merge

@adamretter adamretter force-pushed the hotfix/map-mutability branch 2 times, most recently from b557846 to 3ff7e43 Compare March 8, 2021 03:48
@adamretter adamretter force-pushed the hotfix/map-mutability branch 2 times, most recently from 1a74dd4 to 4af5339 Compare May 16, 2021 16:20
@joewiz joewiz added this to the eXist-6.0.0 milestone May 17, 2021
@line-o line-o mentioned this pull request May 17, 2021
Merged
This was referenced May 27, 2021
Closed
Closed
@adamretter adamretter force-pushed the hotfix/map-mutability branch 3 times, most recently from 8970fd8 to 4ca6ec3 Compare June 28, 2021 21:36
@adamretter adamretter marked this pull request as ready for review June 28, 2021 21:36
@adamretter adamretter mentioned this pull request Jun 28, 2021
Closed
@adamretter adamretter requested a review from joewiz June 28, 2021 21:42
@joewiz joewiz mentioned this pull request Jul 5, 2021
Closed
18 tasks
@adamretter adamretter force-pushed the hotfix/map-mutability branch from 4ca6ec3 to e60e03f Compare August 2, 2021 20:48
This was referenced Aug 2, 2021
Merged
Merged
@adamretter adamretter force-pushed the hotfix/map-mutability branch from e60e03f to 91acbf3 Compare August 3, 2021 12:59
@adamretter adamretter changed the title Further tests and fixes for Maps Switch map:merge from use-last to use-first as per XQuery spec Aug 3, 2021
@eXist-db eXist-db deleted a comment from line-o Aug 3, 2021
@eXist-db eXist-db deleted a comment from line-o Aug 3, 2021
@eXist-db eXist-db deleted a comment from sonarqubecloud bot Aug 3, 2021
@adamretter adamretter requested a review from a team August 3, 2021 13:04
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 3, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@dizzzz
Copy link
Member

dizzzz commented Aug 3, 2021

wait for 6.x

@adamretter adamretter modified the milestones: eXist-6.0.0, eXist-7.0.0 Feb 14, 2022
@alanpaxton alanpaxton force-pushed the hotfix/map-mutability branch from 91acbf3 to 2cbc5a9 Compare May 3, 2022 14:22
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 3, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug E 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

62.0% 62.0% Coverage
0.0% 0.0% Duplication

joewiz
joewiz approved these changes May 12, 2022
View reviewed changes
Copy link
Member

@joewiz joewiz left a comment

Choose a reason for hiding this comment

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

LGTM

@joewiz joewiz mentioned this pull request May 12, 2022
Merged
@adamretter adamretter mentioned this pull request Jun 15, 2022
Merged
@line-o line-o mentioned this pull request Nov 28, 2022
Closed
@line-o
Copy link
Member

line-o commented Nov 28, 2022

This PR adds more than its title and description give away and only the switch to "use-first" by default is breaking.
I would like this PR to be broken up in two parts:

  1. every non-breaking change: support of "reject" and "combine"
  2. the breaking change that switches the default behaviour

Doing this would allow us to merge in the enhancements earlier (prior to exist7) giving users more time to adapt.

@line-o
Copy link
Member

line-o commented Nov 28, 2022 •
edited
Loading

the only commits that are breaking are

@line-o line-o mentioned this pull request Nov 28, 2022
Merged
@line-o
Copy link
Member

line-o commented Nov 28, 2022

#4621 now contains the implementation of options "combine" and "reject" (squashed in to one). When it is merged the last three commits here should be removed and this PR rebased on current develop.

@line-o line-o self-requested a review November 28, 2022 15:21
line-o
line-o reviewed Nov 28, 2022
View reviewed changes
Copy link
Member

@line-o line-o left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We should take extra care that all existing XQuery code that makes use of map:merge#1 is checked. XQSuite is one example but I found many more instances.
This is to not break existing functionality. We might not have tests for this in place in all bundled extensions and the issues might be hard to trace down later.

@line-o line-o self-requested a review November 28, 2022 15:26
line-o
line-o requested changes Nov 28, 2022
View reviewed changes
Copy link
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

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

This code needs to be rebased as it has conflicts regardless if my proposed approach is accepted. (These conflicts are resolved in #4621 already)

@joewiz joewiz added the xquery issue is related to xquery implementation label Jan 3, 2023
@adamretter adamretter force-pushed the hotfix/map-mutability branch from 2cbc5a9 to a8ca76b Compare February 5, 2023 21:34
alanpaxton and others added 2 commits February 5, 2023 23:35
@alanpaxton @adamretter
[refactor] merge duplicates: combine, reject
3e37402 
Reject by counting expected size of merge. Combine uses a merge-with-combine fn from the underlying collection code.
Passes an initial set of tests. There’s some code we have written for completeness (MapType merge with combiner operator) which doesn’t seem to get called. Need to investigate/test further.
@adamretter
[bugfix] Remove duplicated methods
a05df92
@adamretter adamretter force-pushed the hotfix/map-mutability branch from a8ca76b to a05df92 Compare February 5, 2023 23:35
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@reinhapa reinhapa merged commit 23a4823 into eXist-db:develop Feb 6, 2023
@adamretter adamretter deleted the hotfix/map-mutability branch November 28, 2024 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@line-o line-o line-o requested changes

@joewiz joewiz joewiz approved these changes

@dizzzz dizzzz dizzzz approved these changes

Assignees

No one assigned

Labels

enhancement new features, suggestions, etc. xquery issue is related to xquery implementation

Projects

None yet

Milestone

eXist-7.0.0

Development

Successfully merging this pull request may close these issues.

[BUG] behaviour of map:merge#1 is not standards compliant

6 participants

@adamretter @line-o @dizzzz @joewiz @reinhapa @alanpaxton