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

878 Add subsequence-where function #940

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Conversation

michaelhkay
Copy link
Contributor

Supersedes PR #874

Following discussion of PR #874 which proposed an extended subsequence() function with options to define the start and end position by predicates, this new PR proposes instead a subsequence-where() function that allows the start and end position to be defined by predicates, leaving the existing subsequence() function unchanged.

The items-before/after/starting-where/ending-where quartet are dropped.

The new function is inclusive at both ends. To start at the item after the one that matches the start condition, apply tail() to the result. To finish before the item that matches the end condition, apply trunk() to the result.

@ChristianGruen ChristianGruen added the Tests Needed Tests need to be written or merged label Jan 10, 2024
@michaelhkay
Copy link
Contributor Author

There's a use case that this function doesn't handle especially well: "select all items that precede the first "X" if there is one, or all items up to the end otherwise".

@ndw
Copy link
Contributor

ndw commented Jan 23, 2024

The CG agreed to merge this PR at meeting 062

@ndw
Copy link
Contributor

ndw commented Jan 23, 2024

Close #874

@ChristianGruen ChristianGruen added the Blocked PR is blocked (has merge conflicts, doesn't format, etc.) label Jan 24, 2024
@ChristianGruen
Copy link
Contributor

@michaelhkay I assigned »Blocked«, because a conflict must be resolved (due to one of my PRs, sorry).

Regarding the pseudo-code, I noticed that the following query returns an empty sequence:

let $input := 1 to 3
let $from := true#0
let $to := true#0

let $start := index-where($input, $from)[1] otherwise number('INF')
let $end := index-where($input, $to)[. ge $start][1] otherwise number('INF')
return subsequence($input, $start, ($end - $start))

Perhaps it would be more intuitive to return 1 (i.e., to include the first match for which $to returns true).

@ChristianGruen
Copy link
Contributor

I think the informal description of the function is clear enough. Due to the quirky semantics of fn:subsequence, maybe it’s easier to simply drop the equivalent expression.

@michaelhkay
Copy link
Contributor Author

I have revised the PR (a) to remove conflicts, and (b) to take account of comments. The "formal" expansion of the function now relies on fn:slice() rather than fn:subsequence().

@michaelhkay michaelhkay removed the Blocked PR is blocked (has merge conflicts, doesn't format, etc.) label Jan 31, 2024
@ChristianGruen
Copy link
Contributor

@michaelhkay I’m not sure if the version is correct. Examples:

(: returns 6, 7, 8, 9, 10 ? :)
let $input := 1 to 10
let $from := fn { . = 5 }
let $to := fn { . = 5 }
let $start := index-where($input, $from)[1] 
              otherwise count($input) + 1
let $end :=   index-where($input, $to)[. ge $start][1] 
              otherwise count($input) + 1
return slice($input, $start, $end)
(: returns 6, 7 ? :)
let $input := 1 to 10
let $from := fn { . = 5 }
let $to := fn { . = 6 }
let $start := index-where($input, $from)[1] 
              otherwise count($input) + 1
let $end :=   index-where($input, $to)[. ge $start][1] 
              otherwise count($input) + 1
return slice($input, $start, $end)

If it’s correct, I should certainly spend more time on fn:slice first.

@michaelhkay michaelhkay added Tests Added Tests have been added to the test suites and removed Tests Needed Tests need to be written or merged labels Jan 31, 2024
@ndw ndw merged commit d3231a7 into qt4cg:master Feb 5, 2024
2 checks passed
@ChristianGruen
Copy link
Contributor

@ndw I wonder whether the fn:slice code should have been checked first (see my last comment)… Should I open an issue on that?

@ndw
Copy link
Contributor

ndw commented Feb 5, 2024

Yes, open an issue. We agreed to merge this last week and I guess I was on autopilot. I'd have already merged it earlier if there hadn't been a conflict.

@michaelhkay michaelhkay added XQFO An issue related to Functions and Operators Completed PR has been applied, tests written and tagged, no further action needed labels Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Completed PR has been applied, tests written and tagged, no further action needed Tests Added Tests have been added to the test suites XQFO An issue related to Functions and Operators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants