Rename DatePart to DateTimePart #506

am357 · 2022-01-26T20:00:14Z

As DatePart type includes Time components too, rename the type to
reflect the current value range. In addition rename the related
functions.

GitHub Issue: #409

Issue #, if available: 409

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

As DatePart type includes Time components too, rename the type to reflect the current value range. In addition rename the related functions. GitHub Issue: #409

codecov-commenter · 2022-01-26T20:04:27Z

Codecov Report

Merging #506 (e413263) into main (3a7cb56) will not change coverage.
The diff coverage is 87.50%.

@@            Coverage Diff            @@
##               main     #506   +/-   ##
=========================================
  Coverage     82.42%   82.42%           
  Complexity     1329     1329           
=========================================
  Files           171      171           
  Lines         10921    10921           
  Branches       1795     1795           
=========================================
  Hits           9002     9002           
  Misses         1368     1368           
  Partials        551      551

Flag	Coverage Δ
CLI	`24.84% <ø> (ø)`
EXAMPLES	`75.14% <ø> (ø)`
LANG	`84.82% <87.50%> (ø)`
PTS	`∅ <ø> (∅)`
TEST_SCRIPT	`79.68% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lang/src/org/partiql/lang/syntax/TokenType.kt	`100.00% <ø> (ø)`
...partiql/lang/eval/builtins/DateDiffExprFunction.kt	`90.00% <77.77%> (ø)`
lang/src/org/partiql/lang/syntax/SqlParser.kt	`80.09% <81.81%> (ø)`
.../partiql/lang/eval/builtins/ExtractExprFunction.kt	`86.95% <86.66%> (ø)`
.../partiql/lang/eval/builtins/DateAddExprFunction.kt	`92.45% <94.11%> (ø)`
lang/src/org/partiql/lang/errors/ErrorCode.kt	`84.60% <100.00%> (ø)`
...g/src/org/partiql/lang/eval/ExprValueExtensions.kt	`84.71% <100.00%> (ø)`
lang/src/org/partiql/lang/syntax/LexerConstants.kt	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a7cb56...e413263. Read the comment docs.

alancai98

Change looks good overall. Left some minor comments about changing datePart/"date part" to dateTimePart/"date time part" respectively for consistency with this change.

May want to also do a code search for other comments, functions, and variables that need renamed. E.g.

partiql-lang-kotlin/lang/test/org/partiql/lang/eval/EvaluatingCompilerTests.kt

Line 1753 in feb0976

* Ensures that date parts can be used as variable names.

alancai98 · 2022-01-27T00:26:24Z

lang/src/org/partiql/lang/eval/ExprValueExtensions.kt

@@ -100,9 +100,9 @@ fun ExprValue.stringValue(): String =
 fun ExprValue.bytesValue(): ByteArray =
    scalar.bytesValue() ?: errNoContext("Expected LOB: $ionValue", internal = false)

-internal fun ExprValue.datePartValue(): DatePart =
+internal fun ExprValue.datePartValue(): DateTimePart =


May also want to change the name of this extension function to dateTimePartValue()?

alancai98 · 2022-01-27T00:27:53Z

lang/src/org/partiql/lang/eval/builtins/DateAddExprFunction.kt

@@ -67,7 +66,7 @@ internal class DateAddExprFunction(valueFactory: ExprValueFactory) : NullPropaga
                                                      this.hour,
                                                      this.minute,
                                                      this.localOffset)
-            else                -> errNoContext("invalid date part for date_add: ${datePart.toString().toLowerCase()}",
+            else                -> errNoContext("invalid date part for date_add: ${dateTimePart.toString().toLowerCase()}",


Could change error message to say "invalid date time part for ... "

alancai98 · 2022-01-27T00:28:14Z

lang/src/org/partiql/lang/eval/builtins/DateAddExprFunction.kt

@@ -79,12 +78,12 @@ internal class DateAddExprFunction(valueFactory: ExprValueFactory) : NullPropaga

        try {
            val addedTimestamp = when (datePart) {


Variable could also be renamed to dateTimePart and similarly on L79

alancai98 · 2022-01-27T00:29:00Z

lang/src/org/partiql/lang/syntax/LexerConstants.kt

    YEAR, MONTH, DAY, HOUR, MINUTE, SECOND, TIMEZONE_HOUR, TIMEZONE_MINUTE
 }

-internal val DATE_PART_KEYWORDS: Set<String> = DatePart.values()
+internal val DATE_PART_KEYWORDS: Set<String> = DateTimePart.values()


Variable could be renamed to DATE_TIME_PART_KEYWORDS for consistency

alancai98 · 2022-01-27T00:29:25Z

lang/test/org/partiql/lang/eval/builtins/DateAddExprFunctionTest.kt

@@ -73,8 +72,8 @@ class DateAddExprFunctionTest : TestBase() {
            .isExactlyInstanceOf(EvaluationException::class.java)
    }

-    fun parametersForInvalidDatePart() = listOf(DatePart.TIMEZONE_HOUR,
-                                                DatePart.TIMEZONE_MINUTE).map { it.toString().toLowerCase() }
+    fun parametersForInvalidDatePart() = listOf(DateTimePart.TIMEZONE_HOUR,


Function could be renamed to parametersForInvalidDateTimePart

alancai98 · 2022-01-27T00:29:55Z

lang/test/org/partiql/lang/eval/builtins/ExtractEvaluationTest.kt

@@ -83,19 +83,19 @@ class ExtractEvaluationTest : EvaluatorTestBase() {
    data class ExtractFromDateTC(val source: String, val expected: ExprValue?)

    private fun createDateTC(source: String, expected: LocalDate) =
-        DatePart.values()
+        DateTimePart.values()
            .map { datePart ->
                ExtractFromDateTC(
                    source = "EXTRACT(${datePart.name} FROM $source)",
                    expected = when (datePart) {


Variable could be renamed to dateTimePart and same with the variable on L127

abhikuhikar · 2022-01-27T01:58:00Z

I think Alan had already figured out few places where the renaming needs to be done. But there are many more occurrences that might have missed. For example, the documentation - BuiltinFunctions.md also needs to be modified. I would do a simple word search for "datePart" on a project/directory level and also look at the comments and other usages (such as the one in the comment, rightly pointed out by Alan).
I know this is a tedious task. Thanks for taking this up.

am357 · 2022-01-27T16:39:10Z

Good call-outs @abhikuhikar @alancai98, will send a new commit!

See the list of instances: https://paste.debian.net/hidden/9168be70/

am357 · 2022-01-27T19:31:58Z

Changed the rest of occurrences, more details of search result here:
https://paste.debian.net/hidden/9168be70/

abhikuhikar · 2022-01-27T20:06:18Z

Changes LGTM but can you also rename instances of "datePart" and "date part" in BuiltInFunctions.md file?

am357 · 2022-01-27T21:02:49Z

Changes LGTM but can you also rename instances of "datePart" and "date part" in BuiltInFunctions.md file?

That makes sense, will add.

alancai98

LGTM. Mind if we merge this PR after the upcoming branch merge to main? I have some concerns that the files modified here will cause conflicts.

Rename DatePart to DateTimePart

feb0976

As DatePart type includes Time components too, rename the type to reflect the current value range. In addition rename the related functions. GitHub Issue: #409

am357 marked this pull request as ready for review January 26, 2022 20:05

am357 requested review from abhikuhikar and alancai98 January 26, 2022 20:05

alancai98 reviewed Jan 27, 2022

View reviewed changes

Rename the rest of instances of datepart

feaeb34

See the list of instances: https://paste.debian.net/hidden/9168be70/

Update DataTimePart in docs

e413263

alancai98 approved these changes Jan 27, 2022

View reviewed changes

abhikuhikar approved these changes Jan 27, 2022

View reviewed changes

am357 merged commit 73c37e2 into main Jan 27, 2022

alancai98 mentioned this pull request Feb 3, 2022

Merge changes made for Glue Elastic Views #517

Merged

am357 deleted the date-part-to-date-time-part branch February 3, 2022 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename DatePart to DateTimePart #506

Rename DatePart to DateTimePart #506

am357 commented Jan 26, 2022

codecov-commenter commented Jan 26, 2022 •

edited

Loading

alancai98 left a comment

alancai98 Jan 27, 2022

alancai98 Jan 27, 2022

alancai98 Jan 27, 2022

alancai98 Jan 27, 2022

alancai98 Jan 27, 2022

alancai98 Jan 27, 2022

abhikuhikar commented Jan 27, 2022

am357 commented Jan 27, 2022

am357 commented Jan 27, 2022 •

edited

Loading

abhikuhikar commented Jan 27, 2022

am357 commented Jan 27, 2022

alancai98 left a comment

		@@ -79,12 +78,12 @@ internal class DateAddExprFunction(valueFactory: ExprValueFactory) : NullPropaga

		try {
		val addedTimestamp = when (datePart) {

Rename DatePart to DateTimePart #506

Rename DatePart to DateTimePart #506

Conversation

am357 commented Jan 26, 2022

codecov-commenter commented Jan 26, 2022 • edited Loading

Codecov Report

alancai98 left a comment

Choose a reason for hiding this comment

alancai98 Jan 27, 2022

Choose a reason for hiding this comment

alancai98 Jan 27, 2022

Choose a reason for hiding this comment

alancai98 Jan 27, 2022

Choose a reason for hiding this comment

alancai98 Jan 27, 2022

Choose a reason for hiding this comment

alancai98 Jan 27, 2022

Choose a reason for hiding this comment

alancai98 Jan 27, 2022

Choose a reason for hiding this comment

abhikuhikar commented Jan 27, 2022

am357 commented Jan 27, 2022

am357 commented Jan 27, 2022 • edited Loading

abhikuhikar commented Jan 27, 2022

am357 commented Jan 27, 2022

alancai98 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 26, 2022 •

edited

Loading

am357 commented Jan 27, 2022 •

edited

Loading