basedosdados / backend

Backend da BD
https://backend.basedosdados.org/graphql
GNU General Public License v3.0
8 stars 1 forks source link

feat: datetime_range.unit #627

Closed rdahis closed 1 month ago

rdahis commented 2 months ago

Iterando o #621.

laura-l-amaral commented 2 months ago

@rdahis percebi um problema importante com essa implementação que vc sugeriu: temos mais de uma cobertura temporal para tabelas que são parcialmente BD Pro, como que faz nesses casos? acho que vc só tava imaginando um único datetime range para cada tabela né? Vai ficar com uma mesma coluna conectada a 2 datetime_range? Acaba que fica uma informação duplicada e atrapalha a consulta a api

Qual que era o problema da implementação que eu tinha sugerido?

rdahis commented 2 months ago

@rdahis percebi um problema importante com essa implementação que vc sugeriu: temos mais de uma cobertura temporal para tabelas que são parcialmente BD Pro, como que faz nesses casos? acho que vc só tava imaginando um único datetime range para cada tabela né? Vai ficar com uma mesma coluna conectada a 2 datetime_range? Acaba que fica uma informação duplicada e atrapalha a consulta a api

Não é um problema não. Quando tiver múltiplas coverage, pode ficar a mesma coluna ligada à duas. A property table.datetime_range_unit está pegando a "coluna mais comum" entre datetime_ranges. Na prática é pra ser sempre a mesma, porque diferentes coberturas na mesma tabela deveriam ter todas a mesma unidade temporal.

laura-l-amaral commented 2 months ago

@rdahis Ok entendi, podemos seguir assim. O único "problema" é que equipe dados vai ter que preencher igual nas duas e fica aberto para esquecer ou errar e preencher diferente. Ter que preencher 2 vezes a mesma informação não é uma redundância no nosso modelo? Pra lidar com o problema que eu coloquei ali eu incluo no código uma verificação se caso tiver 2 coberturas, se elas estão ambas preenchidas e se estão iguais.

rdahis commented 1 month ago

É uma certa redundância porque acho que nunca teremos uma tabela com duas unidades temporais diferentes (ex: parte com ano e parte com mes). Mas é isso aí mesmo. Eu deixaria passar sem validação mas vou tentar implementar aqui.

Se for crítico para a feature de tradução de tabelas, vamos aprovar logo e adicionar a validação depois? @laura-l-amaral

laura-l-amaral commented 1 month ago

@rdahis não é crítico não pra tradução, é pra gente melhorar o sistema de atualização de metadados só. Ficar mais automático e reduzir os casos que os metadados ficam diferentes dos dados

rdahis commented 1 month ago

Adicionei a validação. Testei local via backend do Django e ele apresenta uma barra de erro quando eu tento adicionar uma segunda unit diferente da primeira. Vejam abaixo. Ele só mostra essa barra vermelha, mas não exibe a mensagem exatamente do erro, que é "Datetime range units do not refer all to the same column.".

Screenshot 2024-07-16 at 13 09 47

De qualquer jeito, seria bom @laura-l-amaral e @jhonylucas74 testarem local antes de aprovar.

rdahis commented 1 month ago

Ainda está aberta essa questão de exibir o erro corretamente no Django. Mas vou dar merge para começarmos a preencher.

rdahis commented 1 month ago

Parece que o deploy não foi. Sabe onde vejo os logs? @jhonylucas74 @gabriel-milan