enable reading of pbfs with node location for ways #111

sergiupantiru · 2023-06-27T07:41:39Z

Fixes #110

core/src/test/resources/com/acervera/osm4scala/primitives/way/way-with-geo

angelcervera

First of all, thanks for your updates and apologies for the delay doing the CR. So busy these days.

Important. I know that there are a lot of comments, but in general (except few things), it looks good to me. ;)

The PR is not passing one of the checkers. Looks like it is not compiling for Scala 2.13.
To reproduce it, you can run PATCH_211=false sbt +test or from the sbt shell:

sbt:osm4scala-root> ++ 2.13.5!
sbt:osm4scala-root> clean
sbt:osm4scala-root> compile

Maybe you can fix as well the warning.

[error] /home/angelcc/projects/tmp/osm4scala/core/src/main/scala/com/acervera/osm4scala/utilities/CoordUtils.scala:52:19: Double does not take parameters
[error]       .doubleValue();
[error]                   ^
[error] /home/angelcc/projects/tmp/osm4scala/core/src/main/scala/com/acervera/osm4scala/utilities/CoordUtils.scala:66:19: Double does not take parameters
[error]       .doubleValue();
[error]                   ^
[warn] /home/angelcc/projects/tmp/osm4scala/core/src/main/scala/com/acervera/osm4scala/model/model.scala:110:48: Widening conversion from Long to Double is deprecated because it loses precision. Write `.toDouble` instead.
[warn]         .drop(1).map(x=> convertToMicroDegrees(x)),
[warn]                                                ^
[warn] /home/angelcc/projects/tmp/osm4scala/core/src/main/scala/com/acervera/osm4scala/model/model.scala:113:49: Widening conversion from Long to Double is deprecated because it loses precision. Write `.toDouble` instead.
[warn]         .drop(1).map(x => convertToMicroDegrees(x))
[warn]                                                 ^
[warn] two warnings found

Anyway, I think that after that, it will fail because the dependency with Spark 3.1.1, but at least, let keep it compatible with Scala 2.13
Don't worry about that. I will fix it before the release.

BTW, I'm thinking about drop Scala 2.11 compatibility because we are not using it and it's a pain for crosscompiling and dependencies, but no idea if other people is still using it. Could you give me a feed back about this?

angelcervera · 2023-07-02T07:52:39Z

core/src/main/scala/com/acervera/osm4scala/utilities/CoordUtils.scala

+   * @param currentValue
+   * @return
+   */
+  def decompressCoord(offSet: Long, delta: Long, currentValue: Double, granularity: Int): Double = {


Maybe these changes are a good idea to increase precision, but are out of the scope of adding the new fields.
Could you move it out of this PR? Before to add it, we need to check if so precision is necessary for the precision level used in OSM and also the performance penalty that we are adding.

BTW, I'm not sure, but I think that the compiler is not going to optimize the code enough so what in fact are constants (like (BigDecimal.valueOf(1E-9))) are going to be created every time you execute the function (so billions times in this case).

So what do you think about remove these changes from this PR and create a new one to talk about it?

i have removed them

...s/spark-utilities/src/test/scala/com/acervera/osm4scala/examples/spark/tagkeys/JobSpec.scala

angelcervera

A part of the compilation error, here another general comment. Could you update the documentation with the new schema and so on. That would be great as well.
If don't have time to do it, I can do it in the future. So don't worry too much.

angelcervera · 2023-07-02T08:12:09Z

spark/src/main/scala/com/acervera/osm4scala/spark/OsmPbfFormat.scala

@@ -122,7 +122,12 @@ class OsmPbfFormat extends FileFormat with DataSourceRegister {

  override def inferSchema(sparkSession: SparkSession,
                           options: Map[String, String],
-                           files: Seq[FileStatus]): Option[StructType] = Some(OsmSqlEntity.schema)
+                           files: Seq[FileStatus]): Option[StructType] =
+    if (options.getOrElse("wayWithGeometry","false").toLowerCase().equals("true")) {


I would keep only one schema.
After this change , I think that I will increase the major version of the library, because back compatibility (and I will take this chance to drop Scala 2.11 comp).
I don't think that adding two nullable columns will affect too much.

Just wanted to keep it backward compatible. I can keep only one if you think it's better

angelcervera · 2023-07-02T08:14:07Z

core/src/main/scala/com/acervera/osm4scala/model/model.scala

@@ -88,7 +89,9 @@ case class WayEntity(
    id: Long,
    nodes: Seq[Long],
    tags: Map[String, String],
-    info: Option[Info] = None
+    info: Option[Info] = None,
+    lat: Seq[Double],


What do you think about adding default values as Seq.empty to simplify migration to the new version? That will make other users happier than if they need to deal with compilation errors. ;)

angelcervera · 2023-07-02T08:17:26Z

spark/src/main/scala/com/acervera/osm4scala/spark/OsmSqlEntity.scala

I would keep this as before, but adding the new two nullable fields. My opinion in the other comment about the schema.

angelcervera · 2023-07-02T08:23:45Z

spark/src/main/scala/com/acervera/osm4scala/spark/OsmPbfRowIterator.scala

-      case FIELD_TAGS      => calculateTags(entity.tags)
-      case FIELD_INFO      => entity.info.map(populateInfo).orNull
-      case fieldName       => throw new Exception(s"Field $fieldName not valid for a Way.")
+    private def populateWay(entity: WayEntity, structType: StructType): Seq[Any] = structType.fields.map(f =>


Sorry, I don't understand this change. Maybe I'm missing something. Why do you prefer .fields over .fieldNames when the only use of field is to get the name so we are adding an extra step in the code? At the end it is exactly the same, but, in my opinion, removing extra step is more readable.

It's the same as for Relations, the type of the field is needed

angelcervera · 2023-07-11T07:07:07Z

Hi @sergiupantiru Tests are still failing. Do you need help?

sergiupantiru · 2023-07-11T07:08:57Z

Hi @sergiupantiru Tests are still failing. Do you need help?

Hey, if you have time I always welcome help. I'm not close to a computer for the next 3 weeks.

angelcervera · 2023-07-12T05:09:51Z

Hi @sergiupantiru Tests are still failing. Do you need help?

Hey, if you have time I always welcome help. I'm not close to a computer for the next 3 weeks.

No rush, take your time.

sonarcloud · 2023-08-10T05:14:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

sergiupantiru · 2023-08-10T05:14:25Z

can you please take another look?

sergiupantiru · 2024-02-12T20:25:58Z

hey, any updates on this?

angelcervera · 2024-02-12T20:44:28Z

@sergiupantiru All looks fine. I remember that I checked it locally as well, but I want to be sure. Next week I will expend time on it and I will create a new release.

enable reading of pbfs with node location for ways

dddb825

sergiupantiru marked this pull request as ready for review June 27, 2023 07:43

angelcervera reviewed Jun 27, 2023

View reviewed changes

core/src/test/resources/com/acervera/osm4scala/primitives/way/way-with-geo Outdated Show resolved Hide resolved

angelcervera requested changes Jul 2, 2023

View reviewed changes

angelcervera reviewed Jul 2, 2023

View reviewed changes

...s/spark-utilities/src/test/scala/com/acervera/osm4scala/examples/spark/tagkeys/JobSpec.scala Show resolved Hide resolved

angelcervera requested changes Jul 2, 2023

View reviewed changes

remove double precision

d3f69ed

revert double precision

96442c8

add default Seq.empty to WayEntity lat and long

f05cb88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable reading of pbfs with node location for ways #111

enable reading of pbfs with node location for ways #111

sergiupantiru commented Jun 27, 2023 •

edited

angelcervera left a comment •

edited

angelcervera Jul 2, 2023 •

edited

sergiupantiru Jul 7, 2023

angelcervera left a comment •

edited

angelcervera Jul 2, 2023

sergiupantiru Jul 7, 2023

angelcervera Jul 2, 2023 •

edited

sergiupantiru Aug 10, 2023

angelcervera Jul 2, 2023

angelcervera Jul 2, 2023

sergiupantiru Jul 7, 2023

angelcervera commented Jul 11, 2023

sergiupantiru commented Jul 11, 2023

angelcervera commented Jul 12, 2023

sonarcloud bot commented Aug 10, 2023

sergiupantiru commented Aug 10, 2023

sergiupantiru commented Feb 12, 2024

angelcervera commented Feb 12, 2024 •

edited

enable reading of pbfs with node location for ways #111

Are you sure you want to change the base?

enable reading of pbfs with node location for ways #111

Conversation

sergiupantiru commented Jun 27, 2023 • edited

angelcervera left a comment • edited

Choose a reason for hiding this comment

angelcervera Jul 2, 2023 • edited

Choose a reason for hiding this comment

sergiupantiru Jul 7, 2023

Choose a reason for hiding this comment

angelcervera left a comment • edited

Choose a reason for hiding this comment

angelcervera Jul 2, 2023

Choose a reason for hiding this comment

sergiupantiru Jul 7, 2023

Choose a reason for hiding this comment

angelcervera Jul 2, 2023 • edited

Choose a reason for hiding this comment

sergiupantiru Aug 10, 2023

Choose a reason for hiding this comment

angelcervera Jul 2, 2023

Choose a reason for hiding this comment

angelcervera Jul 2, 2023

Choose a reason for hiding this comment

sergiupantiru Jul 7, 2023

Choose a reason for hiding this comment

angelcervera commented Jul 11, 2023

sergiupantiru commented Jul 11, 2023

angelcervera commented Jul 12, 2023

sonarcloud bot commented Aug 10, 2023

sergiupantiru commented Aug 10, 2023

sergiupantiru commented Feb 12, 2024

angelcervera commented Feb 12, 2024 • edited

sergiupantiru commented Jun 27, 2023 •

edited

angelcervera left a comment •

edited

angelcervera Jul 2, 2023 •

edited

angelcervera left a comment •

edited

angelcervera Jul 2, 2023 •

edited

angelcervera commented Feb 12, 2024 •

edited