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

Schema: Remove mixed types: road.flags (Parquet compatibility) #135

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

TheRBajaj
Copy link
Contributor

@TheRBajaj TheRBajaj commented Feb 27, 2024

Description

This PR removes the mixed schema types for road.flags property

Before

road.flags could accept multiple types of values that could be put into the same property like array and string values.

After

roads.flags only accepts array property instead of array and string

Reference

https://github.com/OvertureMaps/tf-transportation/issues/57

Testing

Tested using test script

Checklist

  1. Add relevant examples.
  2. Add relevant counterexamples.
  3. Update in-schema documentation using plain English written in complete sentences, if an update is required.
  4. Update Docusaurus documentation, if an update is required.
  5. Review change with Overture technical writer to ensure any advanced documentation needs will be taken care of, unless the change is trivial and would not affect the documentation.

marinrelatic
marinrelatic previously approved these changes Feb 28, 2024
sanjibdutta1
sanjibdutta1 previously approved these changes Feb 28, 2024
Copy link
Collaborator

@vcschapp vcschapp left a comment

Choose a reason for hiding this comment

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

@TheRBajaj need to fix counterexamples.

  1. There are many existing counterexamples that have the old-style flags, but where the intention of those counterexamples isn't to find an error in the road.flags property. Can you please find and fix them all to use new-style flags?
  2. Please get in the habit of looking for counterexamples to fix when doing schema changes!
  3. Could you also add one counterexample that explicitly tests for invalid old-style flags?

@vcschapp
Copy link
Collaborator

@TheRBajaj are you using the pull request template?

Looking at this PR's description, it looks like the template was deleted or trimmed-down. If you're doing that, it's preferable not to.

These are the benefits of the PR template:

  1. It has a standard format, so it makes it easier to find expected information in expected places.
  2. It helps you not forget things.
  3. It has a checklist. Explicitly on the checklist, there's an entry for "Add relevant counterexamples", which would help you remember to do that in this case.

@TheRBajaj TheRBajaj force-pushed the users/rbajajj/remove_mixed_schema_roads_flags branch from 20b6295 to 2233e6d Compare February 29, 2024 23:00
@TheRBajaj TheRBajaj force-pushed the users/rbajajj/remove_mixed_schema_roads_flags branch from 2233e6d to c6ebccd Compare March 1, 2024 02:10
@TheRBajaj TheRBajaj requested a review from vcschapp March 1, 2024 02:11
vcschapp
vcschapp previously approved these changes Mar 1, 2024
Copy link
Collaborator

@vcschapp vcschapp left a comment

Choose a reason for hiding this comment

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

                   __
                  / \--..____
                   \ \       \-----,,,..
                    \ \       \         \--,,..
                     \ \       \         \  ,'
                      \ \       \         \ ``..
                       \ \       \         \-''
                        \ \       \__,,--'''
                         \ \       \.
                          \ \      ,/
                           \ \__..-
                            \ \
                             \ \
                              \ \   :F_P:
                               \ \
                                \ \
                                 \ \
                                  \ \
                                   \ \
                                    \ \

@TheRBajaj TheRBajaj force-pushed the users/rbajajj/remove_mixed_schema_roads_flags branch from c6ebccd to eb1d6ab Compare March 1, 2024 20:39
brad-richardson
brad-richardson previously approved these changes Mar 1, 2024
Copy link
Contributor

@brad-richardson brad-richardson left a comment

Choose a reason for hiding this comment

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

It looks like you have some conflicts to resolve first but LGTM!

sanjibdutta1
sanjibdutta1 previously approved these changes Mar 1, 2024
ibnt1
ibnt1 previously approved these changes Mar 1, 2024
@vcschapp
Copy link
Collaborator

vcschapp commented Mar 1, 2024

It looks like you have some conflicts to resolve first but LGTM!

I see it also. @TheRBajaj could you resolve the conflict to make this PR mergable?

@vcschapp vcschapp dismissed stale reviews from ibnt1, sanjibdutta1, and brad-richardson via 37da66c March 4, 2024 19:21
@vcschapp
Copy link
Collaborator

vcschapp commented Mar 4, 2024

Merging this despite the fact that validation is currently broken. Some cross-PR conflicts caused merging several previous PRs that by themselves were fine to break validation. It will be easier to merge this first, then fix after.

@vcschapp vcschapp merged commit 1d467ca into dev Mar 4, 2024
1 of 2 checks passed
@vcschapp vcschapp deleted the users/rbajajj/remove_mixed_schema_roads_flags branch March 4, 2024 19:25
vcschapp added a commit that referenced this pull request Mar 12, 2024
Co-authored-by: Rahul Bajaj <rbajajj@amazon.com>
Co-authored-by: Victor Schappert <schapper@amazon.com>
jonahadkins pushed a commit that referenced this pull request Jul 12, 2024
Co-authored-by: Rahul Bajaj <rbajajj@amazon.com>
Co-authored-by: Victor Schappert <schapper@amazon.com>
jonahadkins pushed a commit that referenced this pull request Sep 7, 2024
Co-authored-by: Rahul Bajaj <rbajajj@amazon.com>
Co-authored-by: Victor Schappert <schapper@amazon.com>
jonahadkins pushed a commit that referenced this pull request Nov 22, 2024
Co-authored-by: Rahul Bajaj <rbajajj@amazon.com>
Co-authored-by: Victor Schappert <schapper@amazon.com>
jonahadkins pushed a commit that referenced this pull request Dec 4, 2024
Co-authored-by: Rahul Bajaj <rbajajj@amazon.com>
Co-authored-by: Victor Schappert <schapper@amazon.com>
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.

7 participants