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

Search Kit - For "Entity" displays, provide TABLE/VIEW options #31632

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

totten
Copy link
Member

@totten totten commented Dec 19, 2024

Overview

In Search Kit, you can create a saved-search and use it to generate a read-only custom API.

Steps to Reproduce

  • Go to "Search" => "Search Kit" => "New Search"
  • Call it "Grown Ups". Select display name, primary email, and primary phone.
  • Add a search-display of type "DB Entity"
  • Call it "GrownUps"
  • Save

Before

The DB entity is created as a TABLE. This table must be periodically refreshed.

After

You can optionally set a flag to control data-storage for the entity ("MySQL TABLE" vs "MySQL VIEW" vs "MySQL CTE").

Screenshot from 2024-12-19 00-26-28

Technical Details

  • Implementing the _row column on a VIEW requires some trickery.
    • I'm a little skeptical about the performance. Seems like something that should be tested.
    • What are the alternatives? An entity with no primary key? Or make a key using joined values CONCAT(a.id,"_",b.id",...)? Or limit the feature to non-joined searches?
    • Haven't tested, but this trick probably requires MySQL 8.0. (Don't know about Maria versions.)
  • This is currently a draft. I haven't yet written tests or implemented CTE support.
  • In theory, CTE might perform better than VIEW for certain kinds of queries. AFAIK, CTE is more likely to create a (hidden) temporary table (in memory or on disk).
    • That should make it better than VIEW when you need to perform multiple passes over the data (e.g. for analytics -- for aggregate operations).
    • But it may be worse than VIEW if you have (a) large data-sets combined with (b) narrow queries for specific records.
    • It also requires MySQL 8.0. (Don't know about Maria versions.)

Copy link

civibot bot commented Dec 19, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Dec 19, 2024
@totten totten force-pushed the master-live-entity branch from 35499fe to 238e986 Compare December 19, 2024 21:06
public static function getDataModes(): array {
return [
['table'],
['view'],
Copy link
Member Author

Choose a reason for hiding this comment

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

Current test failures are likely because (1) the tests are running on MySQL 5.7 and (2) this new functionality requires MySQL 8.0.

@totten
Copy link
Member Author

totten commented Dec 19, 2024

In the most recent push, I commented out all the references to using Common Table Expressions (CTE) as a mechanism. Some thoughts:

  • VIEWs and CTEs both provide the same essential mechanism of pulling live data -- and letting the DBMS figure out how to integrate the extra lookups into the query plan.
  • There may be cases where VIEWs and CTEs perform differently. But we don't actually know which (if either) of those edges will be more relevant to our userbase. (To use performance as a differentiator, we would need more empirical data...)
  • Structurally, VIEWs are more natural fit in this context:
    • On a conceptual level... the lifecycle of a "DB Entity" and a "SQL VIEW" are similar. You do an upfront design, then save it, and then send many requests to read it. You expect stable schema in those responses.
    • On a code level... it's been relatively elegant to swap CREATE TABLE + INSERT SELECT with CREATE VIEW ... AS SELECT.
    • These newly created entities can be used in searches... i.e. you'd expect them to participate in joins, subqueries, etc. I suspect these follow-on use-cases are a lot easier to support with VIEWs.
  • This is not meant to rule-out CTE-based adapter. There may be reasons to implement that strategy (just maybe not in this PR), eg
    • If we get into a performance scenarios where the CTEs are better.
    • If the structure of "DB Entity" needs to be dynamic (e.g. at runtime, a PHP hook to manipulate the schema of a logical "DB Entity" -- e.g. producing different schemas for different users for the same entity -- that could probably work with CTEs, but our heads might explode)

@totten
Copy link
Member Author

totten commented Dec 20, 2024

Following up on discussion today and consideration of MySQL 5.7 compatibility:

  • I played with defining _row as CONCAT(a.id, '_', b.id, '_', ...) (*for the main the entity and joined entities). This sort of worked in manual interaction, but had some issues. Notes on 2e916db
  • Then I tried dropping the _row entirely. This actually seems to have worked -- at least for basic manual test and for EntityDisplayTest.
    • As written, this would be a contract break. Do we think anything downstream might be reliant on _row? Haven't tested upgrades.
    • If it's a problem, we could try making _row conditional (i.e. defined for TABLE mode but not for VIEW mode). However, some files (like Civi/BAO/SK_Entity) might not be agreeable.
  • Another approach might be to require that the derived entity to pass-through all IDs (_a_id, _contact_1_id, _contact_2_id, etc). And then those could be combined as a composite key. That might get nullify the CONCAT()-vs-GROUP_CONCAT() issue while still allowing a proper key for each row.

@totten
Copy link
Member Author

totten commented Dec 20, 2024

Then I tried dropping the _row entirely... Haven't tested upgrades...

OK, so I tried this upgrade procedure:

  • Install 5.69
  • Create a saved-search (Contact with name, primary email, primary phone). Export as DB Entity.
  • Create another saved-search based on this entity. By default, this displays a column for the _row. Add some more columns. Add a couple displays (Grid and Table).
  • Upgrade to this branch.
  • Open the downstream displays (Grid and Table).

Observations:

  • In the UI, the downstream displays seemed to work -- they simply omitted the _row from output.
  • You could edit these. The fields/columns for _row were presented with blank label, and you could delete the stale item.
  • At the SQL level, the _row column was still present, and no amount of refreshing would make it go away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants