Open seancolsen opened 2 months ago
@mathemancer this is ready for a proper review now.
Oh I also meant to mention that I rebased this on top of #3556 — but only for the sake of convenience during development (so that I could see/run/copy your tests in that PR). I can rebase this back on another branch if you prefer.
Before reviewing this PR, make sure you're familiar with the Propose changes to record summaries for beta.
Examples
These examples use the Library Management data, with the following id values:
"Books"
(oid:36147
)"Title"
(attnum:2
)"Author"
(attnum:10
)"Authors"
"First Name"
(attnum:2
)"Last Name"
(attnum:3
)Auto-generated Books summaries
Manually-templated Books summaries
Thoughts
I feel the least confident about the implementation within
get_record_summaries_via_query
. Very much open to different approaches there, including changing the approach of passing id values in via array.On a similar note, I wrote a test
__SKIP__test_record_summary_performance
. This is skipped because it's failing. The intent of this test is to verify that we're only generating record summaries for the records passed in via the array. But... (sad trombone)... it doesn't actually work like I thought it would. My next step here is to dig into EXPLAIN to get a better sense of the query plan. I thought Postgres would be smarter here, but oh well.This empirical evidence does make me question my original premise with this design. I was hoping that we'd be able to define a bespoke record summary query for a specific table — devoid of any filtering — and then use that at the inner layer to compose the filtered query using application-defined logic. Alas, it may be that the bespoke queries actually need to be aware of the filtering. I'd like to experiment with this more and bounce some ideas around with you.
For the time being, this works. But I'd like to eventually make it faster. In theory we could potentially kick that can down the road.
Are there some types which wouldn't be able to cast to TEXT via
cast( v AS text )
? I don't understand enough about how this works in Postgres. It seems like there is a difference between implicit casting and explict casting, or something like that. I did a little research but didn't find the answers I was looking for. I did write a testtest_record_summary_with_no_text_columns
, which asserts that we can auto generate record summaries for a table having onlyuuid
andpoint
column types. This actually led me to make the fix in 9cca125dbe125784ad51f09fcc99ba09a927023f. I'm hoping this will cover all possible types, but I think you would know better.Checklist
Update index.md
).develop
branch of the repositoryDeveloper Certificate of Origin
Developer Certificate of Origin
``` Developer Certificate of Origin Version 1.1 Copyright (C) 2004, 2006 The Linux Foundation and its contributors. 1 Letterman Drive Suite D4700 San Francisco, CA, 94129 Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed. Developer's Certificate of Origin 1.1 By making a contribution to this project, I certify that: (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. ```