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

On duplicate Source ID, make ALL dups have a number index, e.g. srcId.N #670

Open
sharkAndshark opened this issue May 22, 2023 · 8 comments

Comments

@sharkAndshark
Copy link
Collaborator

sharkAndshark commented May 22, 2023

Does this description reflect Martin's actual actions?
Doc says:

For example, points table will be available at /points, unless there is another source with the same name, or if the table has multiple geometry columns, in which case it will be available at /points.1, /points.2, etc.

I start a Martin server with test postgis database and fetch the catalog API:

[
  {
    "id": "table_source_multiple_geom",
    "content_type": "application/x-protobuf",
    "description": "public.table_source_multiple_geom.geom1"
  },
  {
    "id": "table_source_multiple_geom.1",
    "content_type": "application/x-protobuf",
    "description": "public.table_source_multiple_geom.geom2"
  }
]

It's not table.1 and table.2 but table and table.1

@sharkAndshark sharkAndshark changed the title Does doc reflect actions when it's Multiple Geomtry Column Table in In auto-discover Does doc reflect actions when it's Multiple Geomtry Column Table in auto-discover May 22, 2023
@nyurik
Copy link
Member

nyurik commented May 22, 2023

Agree, we should probably fix the docs, unless we want to change the algorithm. I'm not against changing it, but it might be a bit tricky to implement?

@sharkAndshark
Copy link
Collaborator Author

Yeah. I haven't dug into the code about this part. If I give it a try, may I get your guide? @nyurik

@nyurik
Copy link
Member

nyurik commented May 23, 2023

Sure, but this is not trivial task (but not too difficult either). Current process:

  • All configuration sub-systems call resolve() to convert a name into an ID:
    pub fn resolve(&self, name: &str, unique_name: String) -> String {

    fn resolve_id<T: PgInfo>(&self, id: &str, src_inf: &T) -> String {
    let signature = format!("{}.{}", self.pool.get_id(), src_inf.format_id());
    self.id_resolver.resolve(id, signature)
    }
  • Each source configurator adds that new ID together with the source object to some internal hashmap.
  • Eventually all configurations are joined together in the main resolver:
    pub async fn resolve(&mut self, idr: IdResolver) -> Result<Sources> {

So to modify this, you would need to do the following:

  • modify IdResolver:
    • add a vector of strings to the IdResolver struct, e.g. dup_names: Vec<String>
    • if name already exists, start index from 2 instead of 1
      let mut index: i32 = 1;
    • move index = index.checked_add(1).unwrap(); line to the end of the loop after the match statement
    • if name already exists, but the index is equal to 2 (i.e. there is only one entry in the hashmap, and we are currently adding a 2nd duplicate entry) -- append name.clone() to the self.dup_names
    • introduce a new function to join name with a number, e.g. pub fn make_id(name: &str, index: i32) -> String { format!("{name}.{index}") } -- and use this function instead of the write!(&mut new_name, "{name}.{index}").
  • modify Config::resolve in
    pub async fn resolve(&mut self, idr: IdResolver) -> Result<Sources> {
    • At the very end, after joining, but before the Ok(result) (will need a temp variable to store the joined results), iterate over all idr.dup_names, and remove those names from the joined results, and re-add them under new name by calling make_id(name, 1).

This should pretty much solve the issue, except for one thing -- the internal metadata that gets generated by each source would also need to be updated somehow, e.g. by adding a new source trait method like update_metadata or init_metadata? Basically you would need to delay the tilejson creation until after the ID is known

@sharkAndshark
Copy link
Collaborator Author

Could we update get table source sql first and then call resolve like this resolve(points.1,schema.points.geom1) @nyurik

@nyurik
Copy link
Member

nyurik commented May 23, 2023

@sharkAndshark I'm not sure what you mean. The name -> id conversion happens the same way for SQL tables, SQL functions, mbtiles, and pmtiles sources. There is no need to touch the .sql files at all here

@nyurik nyurik changed the title Does doc reflect actions when it's Multiple Geomtry Column Table in auto-discover On duplicate Source ID, make ALL dups have a number index, e.g. srcId.N Jun 5, 2023
@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Jun 5, 2023

Yeah.You are right.I was thinking group and rename it source.1 source.2 before calling resolve when it's Pg tables.

But it not resolved problems this way.

@nyurik
Copy link
Member

nyurik commented Jun 5, 2023

I'm not too certain this problem is worth the effort at this point. Other issues might be more urgent, like making it very simple to set up all the static resources -- e.g. bring up martin + style + fonts + maplibre lib in one go, or at least document a way to do that easily with other tools, ...

@sharkAndshark
Copy link
Collaborator Author

If Martin could transform and caching fonts between Woff,ttf,sdf.That would be cool.Like there is a ttf in Martin,and client can request pbf(Martin transform them internally).But let's talk about them in another issue/discussion.

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

No branches or pull requests

2 participants