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

DOCS-1639: Recommend BSON date #2582

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

andf-viam
Copy link
Contributor

@andf-viam andf-viam commented Feb 26, 2024

Recommend that MQL query users use the built-in BSON date type instead of converting from string.

  • Usage example include both usages users care about:
    • Using date type for explicit date specified as string to Date() constructor
    • Using Date() constructor without argument to use current timestamp.

Question:

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Feb 26, 2024
@andf-viam andf-viam force-pushed the DOCS-1639-mql-date-query branch from 9061e53 to 46d50cf Compare February 26, 2024 20:01
@viambot
Copy link
Member

viambot commented Feb 26, 2024

Overall readability score: 55.6 (🟢 +0)

File Readability
query.md 57.72 (🔴 -0.54)
View detailed metrics

🟢 - Shows an increase in readability
🔴 - Shows a decrease in readability

File Readability FRE GF ARI CLI DCRS
query.md 57.72 42.92 10 14.1 12.53 6.46
  🔴 -0.54 🔴 -0.3 🔴 -0.31 🟢 +0 🟢 +0.12 🔴 -0.04

Averages:

  Readability FRE GF ARI CLI DCRS
Average 55.6 47.52 10.65 13.22 12.08 7.61
  🟢 +0 🟢 +0 🟢 +0 🟢 +0 🟢 +0 🟢 +0
View metric targets
Metric Range Ideal score
Flesch Reading Ease 100 (very easy read) to 0 (extremely difficult read) 60
Gunning Fog 6 (very easy read) to 17 (extremely difficult read) 8 or less
Auto. Read. Index 6 (very easy read) to 14 (extremely difficult read) 8 or less
Coleman Liau Index 6 (very easy read) to 17 (extremely difficult read) 8 or less
Dale-Chall Readability 4.9 (very easy read) to 9.9 (extremely difficult read) 6.9 or less

@andf-viam andf-viam requested a review from dmhilly February 26, 2024 20:23
@andf-viam
Copy link
Contributor Author

Hi @dmhilly - quick clarification to recommend using the BSON date data type in MQL queries, with an example based on Jack's code as linked in the DOCS ticket. LMK if you'd like to see any changes -- thanks!

Copy link
Member

@dmhilly dmhilly left a comment

Choose a reason for hiding this comment

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

This looks great, thanks Andrew! Left a couple suggestions

@@ -202,6 +202,31 @@ See the [MongoDB Atlas Documentation](https://www.mongodb.com/docs/atlas/data-fe

For information on connecting to your Atlas instance from other MQL clients, see the MongoDB Atlas [Connect to your Cluster Tutorial](https://www.mongodb.com/docs/atlas/tutorial/connect-to-your-cluster/).

#### Query by date

Copy link
Member

Choose a reason for hiding this comment

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

I can link to BSON spec itself if too many MDB docs links already, up to you.

I think using the MongoDB documentation is okay! It has more info than the BSON spec


When using MQL to query your data by date or time range, you can optimize query performance by avoiding the MongoDB `$toDate` expression, using the [BSON `date` type](https://www.mongodb.com/docs/manual/reference/bson-types/#date) instead.

For example, you could use the following to query a dataset over a date range, specifying a target start timestamp in BSON date format, and using the `Date()` constructor to use the current time as the end timestamp:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For example, you could use the following to query a dataset over a date range, specifying a target start timestamp in BSON date format, and using the `Date()` constructor to use the current time as the end timestamp:
For example, you could use the following to query a dataset over a date range, specifying a target start timestamp in BSON date format, and using the JavaScript `Date()` constructor to use the current time as the end timestamp:

Thoughts? Just to make it clear that this is specific to JavaScript/the shell, since to construct a date type in another language they will need to use the language-specific constructs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right on, this is indeed continuing the mongoh example, so JS it is. Clarified this a bit, thank you!

use sensorData

// Set desired start and end times:
const startTime = 'Mon Feb 10 2024 14:45:07 GMT-0500 (Eastern Standard Time)'
Copy link
Member

Choose a reason for hiding this comment

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

Ah I just noticed that this is a string type, not a date. That might confuse users. I don't really know JS but maybe we want something like:

const startTime = new Date('Mon Feb 10 2024 14:45:07 GMT-0500 (Eastern Standard Time)')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right I think, and we can simplify it a bit too if we're using Date() then. Thanks!

@andf-viam andf-viam requested a review from dmhilly February 26, 2024 22:57
Copy link
Member

@dmhilly dmhilly left a comment

Choose a reason for hiding this comment

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

The changes look great - LGTM! 🎉

@viambot
Copy link
Member

viambot commented Feb 28, 2024

You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/2582

@andf-viam andf-viam merged commit d4a2afc into viamrobotics:main Feb 29, 2024
9 of 10 checks passed
@andf-viam andf-viam deleted the DOCS-1639-mql-date-query branch February 29, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants