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

[BEAM-13990] support date and timestamp fields #17404

Merged
merged 2 commits into from Apr 30, 2022

Conversation

reuvenlax
Copy link
Contributor

No description provided.

@reuvenlax
Copy link
Contributor Author

@liu-du I copied your PR and made some changes to integrate it with recent changes - hope that's ok!

@reuvenlax
Copy link
Contributor Author

R: @liu-du

@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #17404 (b11c07b) into master (e6aae70) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #17404   +/-   ##
=======================================
  Coverage   73.85%   73.85%           
=======================================
  Files         690      690           
  Lines       90843    90843           
=======================================
+ Hits        67092    67095    +3     
+ Misses      22538    22535    -3     
  Partials     1213     1213           
Flag Coverage Δ
python 83.65% <ø> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
sdks/python/apache_beam/transforms/combiners.py 93.03% <0.00%> (-0.39%) ⬇️
.../python/apache_beam/ml/inference/sklearn_loader.py
...thon/apache_beam/ml/inference/sklearn_inference.py 92.68% <0.00%> (ø)
...eam/runners/interactive/interactive_environment.py 90.49% <0.00%> (+0.30%) ⬆️
...hon/apache_beam/runners/direct/test_stream_impl.py 94.02% <0.00%> (+0.74%) ⬆️
sdks/python/apache_beam/utils/interactive_utils.py 95.12% <0.00%> (+7.31%) ⬆️

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 e6aae70...b11c07b. Read the comment docs.

@reuvenlax
Copy link
Contributor Author

Run Java PreCommit

@reuvenlax
Copy link
Contributor Author

Run Python PreCommit

@reuvenlax
Copy link
Contributor Author

R: @yirutang

@reuvenlax
Copy link
Contributor Author

friendly ping!

@aaltay
Copy link
Member

aaltay commented Apr 28, 2022

@chamikaramj @yirutang - Could you please review this PR?

@@ -523,14 +523,6 @@ public void testTimePartitioning() throws Exception {
testTimePartitioning(method);
}

@Test
public void testTimePartitioningStorageApi() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT is was just duplicating the above test

@@ -84,10 +143,10 @@ public static class SchemaDoesntMatchException extends SchemaConversionException
.put("NUMERIC", Type.TYPE_STRING) // Pass through the JSON encoding.
.put("BIGNUMERIC", Type.TYPE_STRING) // Pass through the JSON encoding.
.put("GEOGRAPHY", Type.TYPE_STRING) // Pass through the JSON encoding.
.put("DATE", Type.TYPE_STRING) // Pass through the JSON encoding.
.put("DATE", Type.TYPE_INT32)
.put("TIME", Type.TYPE_STRING) // Pass through the JSON encoding.
.put("DATETIME", Type.TYPE_STRING) // Pass through the JSON encoding.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (value instanceof String) {
return value;
} else if (value instanceof BigDecimal) {
return ((BigDecimal) value).toPlainString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.set("timeValue", LocalTime.parse("00:52:07.123456"))
.set("datetimeValue", LocalDateTime.parse("2019-08-16T00:52:07.123456"))
.set("dateValue", LocalDate.parse("2019-08-16"))
.set("numericValue", new BigDecimal("23.4"))
Copy link
Contributor

Choose a reason for hiding this comment

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

bignumeric test?

Copy link
Contributor

Choose a reason for hiding this comment

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

int and double timestamp value test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@reuvenlax
Copy link
Contributor Author

@yirutang comments addressed

.put("timestampvalue", 43L)
.put("timevalue", "00:52:07.123456")
.put("datetimevalue", "2019-08-16T00:52:07.123456")
.put("datevalue", (int) LocalDate.parse("2019-08-16").toEpochDay())
.put("numericvalue", "23.4")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we test repeated byte string? I always had some problems with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yirutang added extra test

@reuvenlax
Copy link
Contributor Author

Run GoPortable PreCommit

@reuvenlax reuvenlax merged commit 58b4d76 into apache:master Apr 30, 2022
jrmccluskey pushed a commit to jrmccluskey/beam that referenced this pull request May 3, 2022
@yirutang
Copy link
Contributor

yirutang commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants