Retrieve function source description from comments #829

sharkAndshark · 2023-08-21T06:57:32Z

If a PostgreSQL function has an SQL comment, it will try to parse as JSON and use its values to override the auto-generated TileJSON. It is recommended to use this form when creating comments to ensure valid JSON values.

DO $do$ BEGIN
    EXECUTE 'COMMENT ON FUNCTION YOUR_FUNCTION (ARG1_TYPE,ARG2_TYPE,..ARGN_TYPE) IS $tj$' || $$
    {
      "description": "description override",
      ...
    }
    $$::json || '$tj$';
END $do$;

Partially implements #822

nyurik · 2023-08-21T16:58:35Z

martin/src/pg/function_source.rs

@@ -13,6 +15,14 @@ use crate::pg::pool::PgPool;
 use crate::pg::PgError::PostgresError;
 use crate::pg::Result;

+#[derive(Serialize, Deserialize, Debug)]


use TileJSON object here - no point in creating a new struct. Simply parse TileJSON from the comment

martin/src/pg/scripts/query_available_function.sql

nyurik · 2023-08-21T17:04:09Z

tests/fixtures/functions/function_Mixed_Name.sql

@@ -13,3 +13,25 @@ RETURNS TABLE("mVt" bytea, key text) AS $$
      WHERE "Geom" && ST_Transform(ST_TileEnvelope("Z", x, y), 4326)
  ) as tile WHERE geom IS NOT NULL) src
 $$ LANGUAGE SQL IMMUTABLE STRICT PARALLEL SAFE;
+
+COMMENT ON FUNCTION "MixedCase"."function_Mixed_Name" (int4,INT4,INT4) IS $tilejson$


See if you can modify these somehow to auto-verified with '{...}'::json approach instead of a unverified string

Weired. PostgreSQL complained syntax error. Need a little bit research on.

COMMENT ON FUNCTION osm_trees(INTEGER,INTEGER,INTEGER) IS ('{"a":1}'::json::TEXT)

Seems it only receive literal value? doc

@sharkAndshark use this method:

DO $do$ BEGIN EXECUTE 'COMMENT ON FUNCTION public.function_null (INT4,INT4,INT4) IS $tj$' || $$ { "minzoom": 0, "maxzoom": 18, "bounds": [-180, -85, 180, 85], "vector_layers": [] } $$::json || '$tj$'; END $do$;

sharkAndshark · 2023-08-22T06:46:32Z

Hi @nyurik Could you give me some suggestions for fixing the errors in SQL files? I've tried many times.
It's weired. The setup-database step in CI failed.

Update:
Seems it's due to the case sentive issue.But I think I shouldn't double quoting every sql file.Fix it and do the method you suggested later

nyurik · 2023-08-22T18:13:43Z

Hi @nyurik Could you give me some suggestions for fixing the errors in SQL files? I've tried many times. It's weired. The setup-database step in CI failed.

Update: Seems it's due to the case sentive issue.But I think I shouldn't double quoting every sql file.Fix it and do the method you suggested later

usually you don't need to quote any identifiers (namespaces, tables, functions and their arguments, etc) in SQL, as long as they don't use any special characters or reserved keywords. BUT, in some very rare cases, someone may have those weird cases. So in our testing, we must make sure Martin supports it properly. And for that, I have a few tests like "MixedCase" namespaces, as well as functions, etc.

Note that the generated SQL automatically always uses quotes identifiers to avoid ambiguity.

nyurik · 2023-08-24T05:34:59Z

I reworked your patch in sharkAndshark#50 -- please take a look and merge if agree.

Basic idea:

tilejson from a comment is stored as a simple json value
when creating the final tilejson, it uses generated tilejson as the "base", and applies all fields from the comment's tilejson as a "patch" (override) using json-patch crate

P.S. I don't think you should add tilejson to ALL unit tests. Instead, add it to just a few, with some variation of which fields are added, and possibly even deleted (set them to null).

sharkAndshark · 2023-08-24T07:01:02Z

Agreed.I realized the tests part and I'm in a conference,merge it later.

sharkAndshark · 2023-08-26T12:58:41Z

To achieve isolation, it is recommended to have one unit test for each observable behavior. Therefore, it might be necessary to remove some comments from the function. However, there is a concern that this level of granularity might be excessive?(And we can not see them on debug.html then) @nyurik I'm looking forward your guide.

nyurik · 2023-08-27T03:02:00Z

@sharkAndshark lets keep things simple :) I think we need to cover these cases. And I think you can do this with just two function comments.

Test 1

set on "MixedCase"."function_Mixed_Name" - to test comments work on weird named functions
override a single field: set the description in a function comment -- this should override the whatever description is set from the full ID automatically
add a new field: set vector_layers field with an array of whatever stuff the function is producing

Test 2

set comment on public.function_zxy_query -- normal name, different param combination
delet a field - in another function comment test, set description to null. No need to set any other fields at all
add some field that is NOT part of tilejson, e.g. "myvalue": { "foo": "bar" } (or whatever similar). This will test that we can add arbitrary complex content

Just doing these two tests, you should be good enough.

sharkAndshark · 2023-08-27T07:44:07Z

I hope the doc is not too bad as English is not my first language and I'm still learning it.

tests/fixtures/functions/function_zxy_query.sql

nyurik · 2023-08-27T16:18:28Z

martin/src/pg/function_source.rs

@@ -33,6 +34,18 @@ pub async fn query_available_function(pool: &PgPool) -> Result<SqlFuncInfoMapMap
            let output_record_names = jsonb_to_vec(&row.get("output_record_names"));
            let input_types = jsonb_to_vec(&row.get("input_types")).expect("Can't get input types");
            let input_names = jsonb_to_vec(&row.get("input_names")).expect("Can't get input names");
+            let tilejson = if let Some(text) = row.get("description") {
+                match serde_json::from_str::<Value>(text) {


parsing now works because it now uses from_str::<Value> instead of from_str::<TileJSON>. Value allows any valid json.

Yeah. hehe. I realized that lol. Cool. ✅

nyurik

great work, thanks!

nyurik · 2023-08-27T16:32:28Z

Thanks @sharkAndshark , i may do another round of docs cleanup later, but this is good to merge for now. Do you perhaps want to do the same thing for tables? I suspect it is possible to add comments to them too.

sharkAndshark · 2023-08-27T16:34:48Z

Yeah sure. I'd love to do that. A issue ticket for discussion first maybe? @nyurik

nyurik · 2023-08-27T16:36:40Z

technically that's the same issue #822 issue - although it is more of an umbrella issue to cover all tilejson modifications by the user. Eventually I think we should just add an extra field to the yaml config, and that field will work the same way as the function comment

sharkAndshark force-pushed the comments branch 4 times, most recently from f1969c3 to 37cb490 Compare August 21, 2023 16:36

nyurik reviewed Aug 21, 2023

View reviewed changes

sharkAndshark force-pushed the comments branch 2 times, most recently from eb929e0 to e786204 Compare August 22, 2023 06:36

sharkAndshark force-pushed the comments branch 2 times, most recently from 95ed89e to 7f6fa16 Compare August 23, 2023 08:04

nyurik mentioned this pull request Aug 24, 2023

Fixed comments #838

Closed

sharkAndshark force-pushed the comments branch from 7665ae7 to b7c2459 Compare August 27, 2023 07:13

sharkAndshark marked this pull request as ready for review August 27, 2023 07:15

nyurik reviewed Aug 27, 2023

View reviewed changes

tests/fixtures/functions/function_zxy_query.sql Outdated Show resolved Hide resolved

Read comment as function description

99d09a9

sharkAndshark force-pushed the comments branch from b7c2459 to 99d09a9 Compare August 27, 2023 16:12

nyurik reviewed Aug 27, 2023

View reviewed changes

nyurik approved these changes Aug 27, 2023

View reviewed changes

nyurik enabled auto-merge (squash) August 27, 2023 16:30

nyurik disabled auto-merge August 27, 2023 16:30

nyurik enabled auto-merge (squash) August 27, 2023 16:30

nyurik merged commit b3fb720 into maplibre:main Aug 27, 2023

nyurik mentioned this pull request Sep 6, 2023

Allow TileJSON SQL comments on Postgres tables #869

Closed

sharkAndshark deleted the comments branch October 7, 2023 03:27

CommanderStorm mentioned this pull request Feb 1, 2025

Set TileJSON layer metadata for function sources in config #822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve function source description from comments #829

Retrieve function source description from comments #829

sharkAndshark commented Aug 21, 2023 •

edited by nyurik

Loading

nyurik Aug 21, 2023

nyurik Aug 21, 2023

sharkAndshark Aug 22, 2023

sharkAndshark Aug 22, 2023 •

edited

Loading

nyurik Aug 22, 2023

sharkAndshark commented Aug 22, 2023 •

edited

Loading

nyurik commented Aug 22, 2023

nyurik commented Aug 24, 2023 •

edited

Loading

sharkAndshark commented Aug 24, 2023

sharkAndshark commented Aug 26, 2023 •

edited

Loading

nyurik commented Aug 27, 2023 •

edited

Loading

sharkAndshark commented Aug 27, 2023

nyurik Aug 27, 2023

sharkAndshark Aug 27, 2023

nyurik left a comment

nyurik commented Aug 27, 2023

sharkAndshark commented Aug 27, 2023 •

edited

Loading

nyurik commented Aug 27, 2023

Retrieve function source description from comments #829

Retrieve function source description from comments #829

Conversation

sharkAndshark commented Aug 21, 2023 • edited by nyurik Loading

nyurik Aug 21, 2023

Choose a reason for hiding this comment

nyurik Aug 21, 2023

Choose a reason for hiding this comment

sharkAndshark Aug 22, 2023

Choose a reason for hiding this comment

sharkAndshark Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

nyurik Aug 22, 2023

Choose a reason for hiding this comment

sharkAndshark commented Aug 22, 2023 • edited Loading

nyurik commented Aug 22, 2023

nyurik commented Aug 24, 2023 • edited Loading

sharkAndshark commented Aug 24, 2023

sharkAndshark commented Aug 26, 2023 • edited Loading

nyurik commented Aug 27, 2023 • edited Loading

Test 1

Test 2

sharkAndshark commented Aug 27, 2023

nyurik Aug 27, 2023

Choose a reason for hiding this comment

sharkAndshark Aug 27, 2023

Choose a reason for hiding this comment

nyurik left a comment

Choose a reason for hiding this comment

nyurik commented Aug 27, 2023

sharkAndshark commented Aug 27, 2023 • edited Loading

nyurik commented Aug 27, 2023

sharkAndshark commented Aug 21, 2023 •

edited by nyurik

Loading

sharkAndshark Aug 22, 2023 •

edited

Loading

sharkAndshark commented Aug 22, 2023 •

edited

Loading

nyurik commented Aug 24, 2023 •

edited

Loading

sharkAndshark commented Aug 26, 2023 •

edited

Loading

nyurik commented Aug 27, 2023 •

edited

Loading

sharkAndshark commented Aug 27, 2023 •

edited

Loading