Skip to content

Optimize MyBible WordsOfChrist fallback query#3671

Open
JyriWee wants to merge 4 commits intoAndBible:current-stablefrom
JyriWee:improve/#2132_mybible_words_of_christ_query_perf
Open

Optimize MyBible WordsOfChrist fallback query#3671
JyriWee wants to merge 4 commits intoAndBible:current-stablefrom
JyriWee:improve/#2132_mybible_words_of_christ_query_perf

Conversation

@JyriWee
Copy link
Contributor

@JyriWee JyriWee commented Feb 28, 2026

What changed

  • Replaced the WordsOfChrist fallback lookup in MyBibleBook.kt to avoid lower(text) on the verses table.
  • Fallback now checks both <J> and <j> directly and keeps LIMIT 1:
    • instr(text, '<J>') > 0 OR instr(text, '<j>') > 0
  • Extracted the SQL into WORDS_OF_CHRIST_MARKUP_QUERY constant.
  • Added unit test to lock query behavior for uppercase/lowercase tags.

Benefits

  • Avoids applying lower() on scanned verse text rows.
  • Keeps fallback detection cheap while preserving case-insensitive <J> tag detection.
  • Prevents regressions with an explicit test.

Possible side effects

  • None functionally intended; detection logic should remain equivalent for <J>/<j> markup.
  • Modules using different/non-standard tags are still not auto-detected (unchanged behavior).

Validation

  • ./gradlew :app:testStandardGithubDebugUnitTest --tests net.bible.service.sword.mybible.MyBibleBookTest --no-daemon
  • ./gradlew :app:assembleStandardGithubDebug -x jsBuild --no-daemon

Follow-up to Copilot review comment on #3670:

Follow up Copilot review on PR AndBible#3670 by removing lower(text) from the fallback scan. Check both <J> and <j> directly and keep LIMIT 1; add a unit test to lock in the query shape.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes the WordsOfChrist fallback detection query in MyBibleBook.kt (a follow-up to #3670). Instead of applying lower(text) over the entire verses table for a case-insensitive <J> tag match, the query now uses two separate instr() checks for <J> and <j> directly, avoiding the per-row lower() function call. The SQL is also extracted into a named constant for reuse and testability.

Changes:

  • Replaced instr(lower(text), '<j>') with instr(text, '<J>') > 0 OR instr(text, '<j>') > 0 in the fallback detection query, and extracted it into WORDS_OF_CHRIST_MARKUP_QUERY
  • Added a unit test to assert the query constant contains the expected instr expressions and limit 1
  • Updated the labeled.yml GitHub Actions workflow to guard the add-to-project step against forks and missing secrets

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
app/src/main/java/net/bible/service/sword/mybible/MyBibleBook.kt Extracts SQL into WORDS_OF_CHRIST_MARKUP_QUERY constant and replaces the inline lower(text) query
app/src/test/java/net/bible/service/sword/mybible/MyBibleBookTest.kt Adds test asserting the constant query contains expected case-handling substrings
.github/workflows/labeled.yml Adds name and an if: guard to prevent the project-add step from running on forks or when the secret is absent

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@tuomas2 tuomas2 added the copilot-approved Copilot review passed or all comments addressed label Mar 3, 2026
@JyriWee
Copy link
Contributor Author

JyriWee commented Mar 3, 2026

Merge-ready summary (2026-03-04):

  • No new review findings after latest fix commit dafab83ae.
  • All review threads are resolved.
  • CI status is green:
    • run-unit-tests
    • run-instrument-tests
  • Local validation already completed for this head:
    • ./gradlew :app:testStandardGithubDebugUnitTest --tests net.bible.service.sword.mybible.MyBibleBookTest -x jsBuild --no-daemon
    • ./gradlew :app:assembleStandardGithubDebug -x jsBuild --no-daemon

From this side PR #3671 is ready for maintainer approval/merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

copilot-approved Copilot review passed or all comments addressed

Projects

Status: Needs triage

Development

Successfully merging this pull request may close these issues.

3 participants