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

Update timestamp desc to unix timestamp milliseconds #138

Conversation

asadowns
Copy link
Contributor

Per #104 and #133 all timestamp references are unix timestamp in milliseconds.

Provide link with different language implementations.

Also, update all provider sample data to be in unix timestamps in milliseconds.

No changes made to agency.

Addresses: #104 and #133

hunterowens and others added 2 commits October 6, 2018 13:02
Per openmobilityfoundation#104 and openmobilityfoundation#133 all timestamp references are unix timestamp in milliseconds.

Provide link with different language implementations.

Also, update all provider sample data to be in unix timestamps in milliseconds.

No changes made to agency.

Addresses: openmobilityfoundation#104 and openmobilityfoundation#133
@thekaveman thekaveman added this to the 0.2.1 milestone Oct 14, 2018
Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Great clarifications, thank you @asadowns!

I'm pinning this to 0.2.1, a downcast from float -> int doesn't seem like a breaking change to me. @hunterowens @ian-r-rose ?

@hunterowens
Copy link
Collaborator

in postgres

CREATE TABLE IF NOT EXISTS test (

float_field float, 
int_field int
);

INSERT INTO test (float_field, int_field) VALUES (2.0, 2.0); 

runs successfully and if you pass 2.0,2.3 it will automatically round floats -> ints.

So at least in the python/postgres DB world this in non-breaking. I don't think that will be universal but right now that is most provider ingest stacks so I am okay making the change.

We should also update the JSONSchema, right @ian-r-rose?

@ian-r-rose
Copy link
Contributor

Yes, I think it should be updated.

I think that, strictly speaking, changing the type from a float to an integer would be considered a breaking change. Python is pretty lax in this regard, and Javascript doesn't even have the concept of integers, but if anybody is developing tooling in JVM-based or C-allied languages there could be some updates required.

As long as we are discussing changing data types, is there a reason the trip_duration, trip_distance, and accuracy are defined as integers rather than floats?

@hunterowens
Copy link
Collaborator

@ian-r-rose can you move that discussion to #128. I'm tracking all JSON schema errors over there.

@thekaveman
Copy link
Collaborator

So we're saying this is breaking then, and should be in 0.3.0?

@thekaveman thekaveman added the Schema Implications for JSON Schema or OpenAPI label Oct 15, 2018
…ta-standard into dev

* 'dev' of https://github.com/CityOfLosAngeles/mobility-data-standard:
  Fixing newlines again (openmobilityfoundation#135)
  typo fix.
  Update README to show associated python libraries (openmobilityfoundation#132)
  Add Lyft
  Add Razor to providers.csv
  update update_vehicle_status to use standard post
  clarification on how we extend GBFS
  Elevating Status of Realtime Feed
@thekaveman thekaveman modified the milestones: 0.2.1, 0.3.0 Oct 16, 2018
@hunterowens
Copy link
Collaborator

@thekaveman

I've been wrestling with this issue for a few days, and learning a whole bunch about how weird python and the concept of time can get.

That being said, this seems like an important change, and I'm unaware of anybody building MDS ingest tooling in a language that doesn't silently downcast floats->ints, so I think we should tag this in 0.2.1.

I'll update the milestone and the JSON schema to get this PR ready to merge.

@hunterowens hunterowens modified the milestones: 0.3.0, 0.2.1 Oct 18, 2018
…://github.com/asadowns/mobility-data-specification into asadowns-adowns/133-104-provider-timestamp-unix-millis

* 'adowns/133-104-provider-timestamp-unix-millis' of https://github.com/asadowns/mobility-data-specification:
  Update timestamp desc to  unix timestamp milliseconds
@hunterowens
Copy link
Collaborator

@asadowns can you give me permission to push to this branch? there are some changes to the JSON schema needed to support this PR. Follow these steps

@asadowns
Copy link
Contributor Author

@hunterowens . Looks like it is already clicked but you should be good to go now.
image

@hunterowens
Copy link
Collaborator

found the issue, was trying to push the @adownsbird to the @asadowns branch. fixed now.

@@ -97,9 +97,10 @@
},
"timestamp": {
"$id": "#/definitions/timestamp",
"title": "Floating-point seconds since Unix epoch",
"title": "Integer seconds since Unix epoch",
Copy link
Collaborator

Choose a reason for hiding this comment

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

seconds -> milliseconds

"type": "number",
"minimum": 0.0
"multiple_of": 1.0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the keyword is multipleOf - but I like this, nice approach!

"Feature"
]
},
"properties": {
"type": "object",
Copy link
Collaborator

Choose a reason for hiding this comment

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

type can be an array, see #142 for how I implemented this to allow null for the optional fields like parking_verification_url.

"type": "number",
"minimum": 0.0
"multiple_of": 1.0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

multipleOf

"type": "number",
"minimum": 0.0
"multiple_of": 1.0,
"minimum": 1483228800
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a good idea to enforce via the schema?

What about a process that generates fake data, shouldn't it be allowed to generate data across whatever time series it wants?

If MDS gets back-ported to TNCs and other related types of shared mobility, this could hamstring the ability to get historic data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that fair, I'm gonna say let's not enforce via schema but some sort of "truth-yness" validator.

"type": "number",
"minimum": 0.0
"multiple_of": 1.0,
"minimum": 1483228800
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment above.

"Feature"
]
},
"properties": {
"type": "object",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use array for type, see comment above

provider/trips.json Outdated Show resolved Hide resolved
provider/trips.json Outdated Show resolved Hide resolved
adownsbird and others added 4 commits October 19, 2018 11:18
Per openmobilityfoundation#104 and openmobilityfoundation#133 all timestamp references are unix timestamp in milliseconds.

Provide link with different language implementations.

Also, update all provider sample data to be in unix timestamps in milliseconds.

No changes made to agency.

Addresses: openmobilityfoundation#104 and openmobilityfoundation#133
@hunterowens
Copy link
Collaborator

@thekaveman changes made.

@thekaveman
Copy link
Collaborator

Closing in favor of #146

@thekaveman thekaveman closed this Oct 19, 2018
@thekaveman thekaveman removed this from the 0.2.1 milestone Nov 25, 2018
@thekaveman thekaveman added this to the 0.3.0 milestone Nov 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Schema Implications for JSON Schema or OpenAPI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants