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

Check for null items in the delete iterator #842

Closed
wants to merge 1 commit into from

Conversation

mr-celo
Copy link
Contributor

@mr-celo mr-celo commented Oct 16, 2023

No description provided.

@mr-celo mr-celo requested a review from a team as a code owner October 16, 2023 11:55
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #842 (bd7c59a) into master (231dd64) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #842   +/-   ##
=======================================
  Coverage   78.77%   78.77%           
=======================================
  Files          48       48           
  Lines        3016     3016           
  Branches      142      132   -10     
=======================================
  Hits         2376     2376           
  Misses        640      640           
Files Coverage Δ
...scala/cognite/spark/v1/AssetHierarchyBuilder.scala 100.00% <100.00%> (ø)

val deletes = rows.map(r => fromRow[DeleteItemByCogniteId](r))
val deletes = rows
.map(fromRow[DeleteItemByCogniteId](_))
.filter(_ != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think pure exposed null is possible here, fromRow macro comes from here https://github.com/cognitedata/cdp-spark-datasource/blob/master/macro/src/main/scala/cognite/spark/compiletime/macros/SparkSchemaHelper.scala
And even it was it doesn't sound too clean to only handle it in this location & without diagnostics to indicate if or when it may happen.

@mr-celo mr-celo marked this pull request as draft October 16, 2023 12:37
@mr-celo mr-celo closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants