-
Notifications
You must be signed in to change notification settings - Fork 24
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
QueryDSL #207
QueryDSL #207
Conversation
import geotrellis.vector._ | ||
|
||
import java.time.ZonedDateTime | ||
|
||
case class LayerLegacy(id: LayerId, metadata: TileLayerMetadata[_], bandCount: Int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have GeoTrellisLayers here, why not to keep all the changes here and after approval port them into GeoTrellis core
"layerName" -> layerId.name, | ||
"zoomLevel" -> layerId.zoom.toString, | ||
"bandCount" -> bandCount.toString | ||
) ++ time.map(t => ("time", t.toString)).toMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that we can add time into attributes in case it is a temporal layer
case (SpatialKeyClass, TileClass) => spatialTileRead | ||
case (SpatialKeyClass, MultibandTileClass) => spatialMultibandTileRead | ||
case (SpatialKeyClass, TileClass) => spatialTileRead | ||
case (SpatialKeyClass, MultibandTileClass) => spatialMultibandTileRead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely feel bad about GeoTrellisRasterSource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of this match or other reasons too ?
core/src/main/scala/geotrellis/store/query/QueryCollection.scala
Outdated
Show resolved
Hide resolved
9ea8811
to
de503be
Compare
} | ||
val points: List[Double] = | ||
if (step == 0) List.empty[Double] | ||
else Range.BigDecimal.inclusive(currentEdge, nextEdge, step).map(_.toDouble).toList |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moradology ^ I changed the syntax due to deprecation warnings
libraryDependencies ++= { | ||
CrossVersion.partialVersion(scalaVersion.value) match { | ||
case Some((2, scalaMajor)) if scalaMajor == 11 => | ||
Seq(circeJava8.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Scala 2.12 java8.time codecs are built in, that is why we have a compat object
4eff2d5
to
c754bf7
Compare
implicit class ProjectedExtentOps(self: ProjectedExtent) { | ||
def intersects(projectedExtent: ProjectedExtent): Boolean = self.extent.intersects(projectedExtent.reproject(self.crs)) | ||
def covers(projectedExtent: ProjectedExtent): Boolean = self.extent.covers(projectedExtent.reproject(self.crs)) | ||
def contains(projectedExtent: ProjectedExtent): Boolean = self.extent.contains(projectedExtent.reproject(self.crs)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pomadchin think these are worth just moving to GT main after this? Seems a shame that we still don't have a way to abstract over both extent and ProjectedExtent but also a shame to have to write utility code in apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@echeipesh 💯 agree, along with all GeoTrellisRasterSources enhancements that are implemented here and probably with QueryDSL as well
LayerExtent(IO.pure(algebra), IO.pure(simpleLayers), ConcurrentInterpreter.DEFAULT) | ||
} | ||
}.getOrElse(throw new Exception("bad interpreter")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a better exception message. I believe this will show up in response to request that can't be satisfied (ie: the layer name doesn't produce any sources). Does this error filter back up to WCS response? (I'll come back to double check).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're correct; Originally it was just unsafe .get
from a map & probably it deserves a separate issue to walk though all the exceptions we throw in the code of gt-server?
val simpleLayers = | ||
layerDefinitions.collect { case ssc@SimpleSourceConf(_, _, _, _) => ssc.model } | ||
layerDefinitions.collect { case ssc @ SimpleSourceConf(_, _, _, _) => ssc.models }.flatten |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it may result in each layer being listed once per time-stamp in GetCapabilities ?
case KeyBounds(minKey, maxKey) => | ||
(minKey.time.toInstant.toEpochMilli to maxKey.time.toInstant.toEpochMilli by temporalResolution).toList.map { time => | ||
new GeoTrellisRasterSourceLegacy(attributeStore, dataPath, sourceLayers, Some(ZonedDateTime.ofInstant(time, ZoneOffset.UTC)), targetCellType) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This unfortunately is not correct for temporal layers. There could be time buckets that have no records stored in them and there could be time buckets which have multiple records stored in them. The only way to know which is which is to have use store the used values in metadata. Thinking of a concrete suggestion for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@echeipesh provide time as an input into this build function? (this can be a part of configuration)
But what to do if you don't know anything about the temporal layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't really report anything you don't know. The alternative is that you report a date for which there is no data and query comes back empty.
case (SpatialKeyClass, TileClass) => spatialTileRead | ||
case (SpatialKeyClass, MultibandTileClass) => spatialMultibandTileRead | ||
case (SpatialKeyClass, TileClass) => spatialTileRead | ||
case (SpatialKeyClass, MultibandTileClass) => spatialMultibandTileRead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of this match or other reasons too ?
@JsonCodec case class Between[A](from: ZonedDateTime, to: ZonedDateTime, fieldName: String = "time") extends QueryF[A] | ||
@JsonCodec case class WithName[A](name: String) extends QueryF[A] | ||
@JsonCodec case class WithNames[A](names: Set[String]) extends QueryF[A] | ||
@JsonCodec case class Nothing[A]() extends QueryF[A] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps None
is a better complement to All
? Nothing
seems to me like literally 'no thing' whereas this is just none of the instances of w.e particular thing we're talking about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moradology None
is a reserved word; and the none
function potentially can confuse users. I'm happy to try Any / Nothing though but probably any
can also confuse users seeing that as a part of a Query DSL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:yoda:
@@ -25,7 +25,7 @@ import geotrellis.store.{GeoTrellisPath, GeoTrellisRasterSourceLegacy} | |||
* a geotrellis-contrib [[RasterSource]]. | |||
*/ | |||
sealed trait RasterSourceConf { | |||
def toRasterSource: RasterSource | |||
def toRasterSources: List[RasterSource] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a significant alteration, but I couldn't figure out what it was for (the only implementation I saw here just attached the lone RS to Nil
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was to make use of the Repository interface. The whole thing was to make queriable temporal layers and a temporal layer is just a set of RasterSources. So having a List of RasterSources read from the configuration can allow us to create an instance of a Repository that we can query by whatever is supported by the dsl and by time as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enough for now. We will fix the behavior for querying space-time layers in next step.
ac1c4b5
to
b5daf5e
Compare
Overview
This PR adds a concept of RasterSource catalog that is possible to query via they special query DSL
Checklist
Closes #162
Closes #163
Closes #204